Opened 9 years ago
Closed 9 years ago
#11737 closed defect (fixed)
Editor CSS problems
Reported by: | Katie Vance | Owned by: | Jared Jurkiewicz |
---|---|---|---|
Priority: | high | Milestone: | 1.5.1 |
Component: | Editor | Version: | 1.5 |
Keywords: | Cc: | Katie Vance | |
Blocked By: | Blocking: |
Description
In RTL, the font drop down has a border next to the arrow where there is no border in LTR. This happens in all themes.
All disabled editors do not show the font drop down arrows in claro, this is not just an RTL problem.
Attachments (11)
Change History (45)
Changed 9 years ago by
Attachment: | editorBorderAndImageRtl.png added |
---|
Changed 9 years ago by
Attachment: | eidtor_css.patch added |
---|
comment:1 Changed 9 years ago by
comment:2 Changed 9 years ago by
Milestone: | tbd → 1.5.1 |
---|
comment:3 Changed 9 years ago by
The .dijitEditorDisabled reference is too restricted, since that rule should presumably apply to any disabled ComboBox in a Toolbar, it could be
.claro .dijitToolbar .dijitComboBoxDisabled .dijitArrowButtonInner { background-position:0 50%; }
comment:4 Changed 9 years ago by
The problem is a little more complicated than CSS, as it turns out. Though they appear disabled, the dropdowns are not actually disabled. This is because the setDisabledAttr api in editor is firing before all the plugins have loaded, rendering the state 'odd'. IT actually needs to wait until the plugins have all loaded before firing, so it needs to wait on the same deferred as setting the value does, as the editor needs to be basically completely initialized. I'm attaching two patches, one for trunk, one for 1.5.1, that resolves the state load issue. Really need a better disable API for plugins. :)
comment:5 Changed 9 years ago by
comment:6 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
Changed 9 years ago by
Attachment: | after22885.gif added |
---|
screenshot of display problem after [22885]
comment:7 Changed 9 years ago by
no need to check "this.setValueDeferred.fired < 0", if a deferred already fires, addCallback will call the callback immediately
comment:8 Changed 9 years ago by
Component: | General → Editor |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
Thanks for the work on this. There are some issues though.
- Like Cougar said:
var disableFunc = ...; if(this.setValueDeferred && this.setValueDeferred.fired < 0){ this.setValueDeferred.addCallback(disableFunc); }else{ disableFunc(); }
can be simplified to:
this.setValueDeferred.addCallback(...);
(The change to use a Deferred makes sense. I can see in themeTester in 1.5 how the ComboBox is enabled on the (initially) disabled Editor.)
- the CSS is still incorrect, as I wrote above in comment:3. It needs to apply to all disabled ComboBox's in Toolbars, not just for editor.
- it looks wrong, there's a white line in the gray background:
- why does this need to go into the 1.5 branch? Is it a critical regression? It doesn't seem like it.
comment:10 Changed 9 years ago by
Owner: | changed from Jared Jurkiewicz to Katie Vance |
---|---|
Status: | reopened → new |
It should go into the branch because disabling the editor is effectively broken without the fixes to delay disablement.
Updated css/files in: [22892] and [22891] I could swear I removed that editor css selector several times. Not sure why it showed up yet again (*sigh*)
Reassigning to ksv to continue her work on fixing the white border line.
Changed 9 years ago by
Attachment: | eidtor_css_white_line.patch added |
---|
Attaching css fix for the white line.
Changed 9 years ago by
Attachment: | afterClick.JPG added |
---|
screenshot to show onclick event was fired on a disabled combobox
comment:11 Changed 9 years ago by
Owner: | changed from Katie Vance to Jared Jurkiewicz |
---|
I attached a fix for the extra white line showing up. However we still have an issue with the combo box not getting completely disabled. If you click on the disabled box (not the arrow drop down) it's onclick event is still causing the box to turn white. I'm guessing this should be fixed in the plugin?
This is what it looks like after you click on it: http://bugs.dojotoolkit.org/attachment/ticket/11737/afterClick.JPG
comment:12 Changed 9 years ago by
Katie - presumably after that new patch the changes to dijit_rtl.css in [22884] and [22885] are no longer needed?
Jared - the CSS changes in [22891] and [22892] look incorrect since they are affecting both enabled and disabled ComboBox, aren't they just supposed to be for a disabled ComboBox?
Also, this shouldn't be in 1.5.1. Disabling the editor is working in 1.5.0, even creating an initially disabled editor. (The editor contents are not editable, which is the main point of disabling.) The only issue is that in the latter case (creating an initially disabled editor) the ComboBox is still focusable. That's hardly a critical bug. Is there some other issue?
comment:13 Changed 9 years ago by
PS: oh maybe the dijit_rtl.css changes are still needed for tundra/soria/nihilo (RTL mode)?
Also, it seems like the !important in that dijit_rtl.css rule will override the border: none in the attachment:eidtor_css_white_line.patch, are you sure that's working in RTL mode? If the themes are designed to only specify border-color, not border-width, leaving dijit.css and dijit_rtl.css to specify border-width, then probably you don't need !important anywhere.
comment:14 Changed 9 years ago by
The patch I sent is not cumulative. All patches should be put together.
Unfortunately we need both border css statements in dijit_rtl.css and Common.css because they are applied to different dom nodes. The Common.css setting applies to the input node because of .dijitArrowButtonInner and the dijit_rtl applies to it's parent node because of .dijitArrowButtonContainer. So the latest patch should be added to the other patches. I've tested the patch in both directions and it is working. The !important is needed to override the other !important setting. The only other option there is to change the border color to be the same as the background (so you can't see it), but that may not be possible without putting a separate rule for every theme.
comment:15 Changed 9 years ago by
You can click on the dropdown and it does something (Dropdown opens, you can pick a font, etc). It's also likely to invoke 'execCommands' on a disabled editor, which may or may not throw errors (I haven't checked). Having enabled buttons in a disabled editor runs risks of it firing/trying to do something on disabled content.
I feel it's important to fix this in 1.5.1 as well, to avoid possible scenarios as noted above. It isn't just FontChoice? that could be affected, any plugin that isn't properly disabled could trigger bad side-effects.
comment:16 Changed 9 years ago by
You're right on the other issues (Re, css), I'll look at them when I have time (which I haven't had a lot lately). I'll do what I can.
comment:20 Changed 9 years ago by
Applied all current patches. I really do fundamentally believe this should be in 1.5.1 as well. The ramifications are more than just FontChoice? (It's just one we see). The failure to properly disable plugin buttons and such in a user-built plugin has the change of causing failures should someone exec one by accident while the editor is in disabled state. The fix for it is minor (using the deferred delay).
comment:21 Changed 9 years ago by
If after all that I have explained is not enough to convince you, Bill, I'll pull the changes from 1.5.1. I do really believe it's a bigger issue (since it affects more than just combobox), but you are in charge of dijit.
So, if you say pull it, I'll pull it.
comment:22 Changed 9 years ago by
comment:23 Changed 9 years ago by
If you want to, go for it. Also notice that the editor body goes all white too, so I don't think it's strictly the plugin.
- Jared
comment:24 Changed 9 years ago by
I do have to wonder if removing those 'white' borders is correct. If you look in the themeTester at the form widgets page, the disabled comboboxes have the white outlines.
comment:25 Changed 9 years ago by
Bill:
Cases that will break without the disable being correct in 1.5:
ViewSource?: If this isn't disabled correctly, toggling it will throw errors/mess up the editor.
FullScreen?: Same as above. If not properly disabled, the code to size out will break.
So any table instantiated with these and initially set disabled will have issues without the fix being in 1.5.
comment:26 Changed 9 years ago by
Hi Jared - I tried modifying test_ViewSource.html to set the editor initially disabled. After loading the page, enabling the editor (via dijit.byId("editor0").set("disabled", false)
and pressing the view source button it seemed to work fine. Can you tell me how to reproduce the problem? (I was trying on FF/mac.)
Katie -OK, I see about those CSS rules being on different nodes, nevermind :-). Thanks for looking at that click problem.
I do have to wonder if removing those 'white' borders is correct. If you look in the themeTester at the form widgets page, the disabled comboboxes have the white outlines.
I see your point, but ComboBox in a toolbar looks quite different from a normal ComboBox (outside of a Toolbar). A vanilla enabled ComboBox has a black separating line, then white padding around the button, then a gray button (with a black arrow), whereas the Editor Toolbar enabled ComboBox has no black line, no white padding, and no grayness for the button. It's all white except for the black arrow. So I figure that the disabled Editor Toolbar ComboBox should also be a single color except for the arrow itself.
Maybe there are some subtleties that I'm not seeing on my monitor.
comment:27 Changed 9 years ago by
You don't enable it. The WiewSource? will already be enabled because it was not properly disabled. It only looks disabled, but if you hover over it, it shows an active hover box. Furthermore, you can click it.
I tried this, and I could still click on viewSource in the editor and do stuff. In fact, I could chance the editor contents (I had expected errors, but the internals of the editor seem intact enough it doesn't throw). In any event, this shows that editor is not completely disabled.
See attached screenshots.
Without disabled waiting until all plugins have initialized to disable them, you end up with states/cases people can modify/do things they shouldn't be able to. The attached screenshots bear this out.
Changed 9 years ago by
Attachment: | Disabled_Editor_Active_Buttons.png added |
---|
A supposedly disabled editor with active buttons.
Changed 9 years ago by
Attachment: | Disabled_Editor_Allowing_ViewSource.png added |
---|
A disabled editor allowing view source to be activated.
Changed 9 years ago by
Attachment: | Changed_Disabled_editor.png added |
---|
A disabled editor, through the use of view source, showing it being altered.
comment:29 Changed 9 years ago by
Sorry I wasn't articulating my concerns very well, Bill. After having been in Editor for so long, the issue seemed worse than the defect text stated. Glad I was able to make a clearer case as to why it's pretty bad.
As for the whole click/white thing that ksv is looking at. How does editor mask itself completely grey as it is? It seems to be some sort of focus issue with editor, as clicking on the editor body flips it to whitish too (not just the dropdowns).
comment:30 Changed 9 years ago by
It appears that the filteringSelect only turns white in claro. The editor turning white happens in all themes.
comment:31 Changed 9 years ago by
Owner: | changed from Jared Jurkiewicz to Katie Vance |
---|
Reassigning back to ksv since she is working on the 'white when clicked' issue.
comment:32 Changed 9 years ago by
Owner: | changed from Katie Vance to Jared Jurkiewicz |
---|
Attached patch for the problems with the input box and the editor turning white when a user clicks on the disabled drop down plugin.
Changed 9 years ago by
Attachment: | editor_css_white_background.patch added |
---|
previous patch was corrupted, try this one
comment:34 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
Jared, I attached a patch for this issue.