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

http://download.dojotoolkit.org/release-1.5.0/dojo-release-1.5.0/dijit/themes/themeTester.html?theme=claro&dir=rtl

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)

editorBorderAndImageRtl.png (23.3 KB) - added by Katie Vance 9 years ago.
eidtor_css.patch (1.2 KB) - added by Katie Vance 9 years ago.
editor_disable_trunk.patch (4.1 KB) - added by Jared Jurkiewicz 9 years ago.
Trunk patch
editor_disable_1.5.patch (4.1 KB) - added by Jared Jurkiewicz 9 years ago.
Editor 1.5 patch
after22885.gif (6.8 KB) - added by bill 9 years ago.
screenshot of display problem after [22885]
eidtor_css_white_line.patch (484 bytes) - added by Katie Vance 9 years ago.
Attaching css fix for the white line.
afterClick.JPG (1.4 KB) - added by Katie Vance 9 years ago.
screenshot to show onclick event was fired on a disabled combobox
Disabled_Editor_Active_Buttons.png (192.6 KB) - added by Jared Jurkiewicz 9 years ago.
A supposedly disabled editor with active buttons.
Disabled_Editor_Allowing_ViewSource.png (201.7 KB) - added by Jared Jurkiewicz 9 years ago.
A disabled editor allowing view source to be activated.
Changed_Disabled_editor.png (172.3 KB) - added by Jared Jurkiewicz 9 years ago.
A disabled editor, through the use of view source, showing it being altered.
editor_css_white_background.patch (1018 bytes) - added by Katie Vance 9 years ago.
previous patch was corrupted, try this one

Download all attachments as: .zip

Change History (45)

Changed 9 years ago by Katie Vance

Attachment: editorBorderAndImageRtl.png added

Changed 9 years ago by Katie Vance

Attachment: eidtor_css.patch added

comment:1 Changed 9 years ago by Katie Vance

Jared, I attached a patch for this issue.

comment:2 Changed 9 years ago by Jared Jurkiewicz

Milestone: tbd1.5.1

comment:3 Changed 9 years ago by bill

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 Jared Jurkiewicz

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. :)

Changed 9 years ago by Jared Jurkiewicz

Attachment: editor_disable_trunk.patch added

Trunk patch

Changed 9 years ago by Jared Jurkiewicz

Attachment: editor_disable_1.5.patch added

Editor 1.5 patch

comment:5 Changed 9 years ago by Jared Jurkiewicz

(In [22884]) Fixing setting of disabled state too early. \!strict refs #11737

comment:6 Changed 9 years ago by Jared Jurkiewicz

Resolution: fixed
Status: newclosed

(In [22885]) Fixing setting of disabled state too early. \!strict fixes #11737

Changed 9 years ago by bill

Attachment: after22885.gif added

screenshot of display problem after [22885]

comment:7 Changed 9 years ago by liucougar

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 bill

Component: GeneralEditor
Resolution: fixed
Status: closedreopened

Thanks for the work on this. There are some issues though.

  1. 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.)

  1. 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.
  1. it looks wrong, there's a white line in the gray background:

screenshot of display problem after [22885]

  1. why does this need to go into the 1.5 branch? Is it a critical regression? It doesn't seem like it.

comment:9 Changed 9 years ago by Katie Vance

We not need this fixed in 1.5, it is not critical.

comment:10 Changed 9 years ago by Jared Jurkiewicz

Owner: changed from Jared Jurkiewicz to Katie Vance
Status: reopenednew

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 Katie Vance

Attachment: eidtor_css_white_line.patch added

Attaching css fix for the white line.

Changed 9 years ago by Katie Vance

Attachment: afterClick.JPG added

screenshot to show onclick event was fired on a disabled combobox

comment:11 Changed 9 years ago by Katie Vance

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 bill

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 bill

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 Katie Vance

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 Jared Jurkiewicz

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 Jared Jurkiewicz

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:17 Changed 9 years ago by Jared Jurkiewicz

(In [22894]) Applying further css updates. refs #11737

comment:18 Changed 9 years ago by Jared Jurkiewicz

(In [22895]) Applying further css updates. refs #11737

comment:19 Changed 9 years ago by Jared Jurkiewicz

(In [22896]) Applying further css updates. refs #11737

comment:20 Changed 9 years ago by Jared Jurkiewicz

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 Jared Jurkiewicz

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 Katie Vance

Jared, those 3 fixes are good, however, we still have the issue I mentioned above. If you click on the disabled box (not the arrow drop down) it's onclick event is still causing the box to turn white. Should I look to fix this at the plugin level? It happens to all plugins with a drop down box:

screenshot to show onclick event was fired on a disabled combobox

comment:23 Changed 9 years ago by Jared Jurkiewicz

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 Jared Jurkiewicz

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 Jared Jurkiewicz

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 bill

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 Jared Jurkiewicz

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 Jared Jurkiewicz

A supposedly disabled editor with active buttons.

Changed 9 years ago by Jared Jurkiewicz

A disabled editor allowing view source to be activated.

Changed 9 years ago by Jared Jurkiewicz

Attachment: Changed_Disabled_editor.png added

A disabled editor, through the use of view source, showing it being altered.

comment:28 Changed 9 years ago by bill

Ah I see, OK in that case I'm OK with it.

comment:29 Changed 9 years ago by Jared Jurkiewicz

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 Katie Vance

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 Jared Jurkiewicz

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 Katie Vance

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 Katie Vance

previous patch was corrupted, try this one

comment:33 Changed 9 years ago by Jared Jurkiewicz

(In [22933]) Fix for the editor going white in claro. refs #11737

comment:34 Changed 9 years ago by Jared Jurkiewicz

Resolution: fixed
Status: newclosed

(In [22934]) Final bit of the fix for disabling the editor. Claro issue resolved. fixes #11737

Note: See TracTickets for help on using tickets.