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

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Comment 2

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 3

[Dão Gottwald [:dao]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Neil Deakin]

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)

Comment 6

[[e-mail]]

(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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 8

[Mike Connor [:mconnor]]

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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