Closed Bug 482585 Opened 12 years ago Closed 12 years ago

[FIX]Chrome styles cause all content HTML class changes to restyle

Categories

(Toolkit :: XUL Widgets, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

We have some rules in xul.css that match all nodes in the HTML namespace with a given class name.  This means that any change to a class attribute in any webpage triggers a restyle of that node.  I discovered this when removing the "reftest-wait" class caused a full restyle of my reftest and actually changed the rendering in the process.

I see a few obvious fixes here:

1)  Restrict the styles to the input/textarea elements we really want to match
    here.  This still means they'll affect web pages, though (e.g. a content
    HTML input with the right class will trigger the styles).
2)  Move the rules into (non-themed) scoped stylesheets in the relevant XBL
    widgets.

I'm really tempted to do the latter.  Any obvious issues with it?
Attached patch Proposed fixSplinter Review
Attachment #366642 - Flags: superreview?(dbaron)
Attachment #366642 - Flags: review?(mconnor)
Attachment #366642 - Flags: review?(enndeakin)
Attachment #366642 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 366642 [details] [diff] [review]
Proposed fix

>diff --git a/toolkit/content/textbox.css b/toolkit/content/textbox.css

>+textbox[empty="true"] html|*.textbox-input ,
>+textbox[empty="true"] html|*.textbox-textarea {
>+  text-align: left;
>+  direction: ltr;
>+}
>+
>+textbox[empty="true"][chromedir="rtl"] html|*.textbox-input ,
>+textbox[empty="true"][chromedir="rtl"] html|*.textbox-textarea {
>+  text-align: right;
>+  direction: rtl;
>+}
>+
>+textbox[empty="true"] html|*.textbox-input[emptytextdelay="true"] ,
>+textbox[empty="true"] html|*.textbox-textarea[emptytextdelay="true"] {
>+  color: transparent !important;
>+}

Do these actually work from within the scoped stylesheet?
I think they do, but is <stylesheet src="chrome://global/content/textbox.css"/> really needed for bindings that extend chrome://global/content/bindings/textbox.xml#textbox? At that point, we could as well update all textbox-inputs and textbox-textareas to inherit @empty and @chromedir. That wasn't done originally because I didn't want to force all extended textboxes with custom content (including those that aren't in toolkit) to do the same.
> Do these actually work from within the scoped stylesheet?

I used this testcase to double-check that (which my build with this patch passes):

<?xml version="1.0"?>
<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
  <textbox id="x" empty="true" chromedir="rtl" value="aaa"/>
  <textbox id="y" multiline="true" empty="true" chromedir="rtl" value="bbb"/>
  <script>
    setTimeout(function() {
      document.getElementById("x").setAttribute("chromedir", "rtl");
      document.getElementById("y").setAttribute("chromedir", "rtl");
    }, 1000);
  </script>
</window>

(have to reset the chromedir because onload clobers it, apparently).

> is <stylesheet src="chrome://global/content/textbox.css"/> really needed for
> bindings that extend chrome://global/content/bindings/textbox.xml#textbox

Er, no.  I just added it to all the places that used the class but weren't in the same file, but I think you're right: numberbox and the two autocomplete bindings don't actually need to include that stylesheet directly.  datetimepicker does, though.
Blocks: 482592
Component: Themes → XUL Widgets
QA Contact: themes → xul.widgets
Comment on attachment 366642 [details] [diff] [review]
Proposed fix

As discussed, the autocomplete and numberbox should just inherit the stylesheets.

Also the xpfe change is for suite/seamonkey, but you didn't update the version of xul.css nor add the stylesheets to it. I wouldn't bother though and I'd just drop that change and/or ask the other Neil if any changes are needed (it's possible the file isn't being used anymore)
Attachment #366642 - Flags: review?(enndeakin) → review+
(In reply to comment #5)
> Also the xpfe change is for suite/seamonkey, but you didn't update the version
> of xul.css nor add the stylesheets to it. I wouldn't bother though and I'd just
> drop that change and/or ask the other Neil if any changes are needed (it's
> possible the file isn't being used anymore)
I'd certainly want autocomplete.xml to be patched everywhere xul.css is, otherwise (for instance) the Thunderbird 3.next builds will stop working.
> Also the xpfe change is for suite/seamonkey, but you didn't update the version
> of xul.css nor add the stylesheets to it.

Seamonkey uses the same xul.css as Firefox, from the same place (toolkit), last I checked.  It's certainly the only xul.css in the comm-central tree.

In any case, the point is moot since that xpfe binding inherits from textbox.xml#textbox.  I'll remove the includes from both autocomplete bindings and numberbox, as described.

Should I still get mconnor's review, or am I good to go here?
I think this is good to go, RSI has me on limited typing, so wasn't going to get to this until Friday at least.
Attachment #366642 - Flags: review?(mconnor)
Pushed http://hg.mozilla.org/mozilla-central/rev/27cbf8dd3fba.

No test offhand, though the tests for the followup bug don't fail when they should have without this fix...  I don't have a good way to test that, though.  :(
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.