Closed Bug 1414544 Opened 4 years ago Closed 4 years ago

In-content prefs: XBL compat hack matched, please file a bug blocking bug 1374247. Selector: caption

Categories

(Thunderbird :: Preferences, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 58.0

People

(Reporter: Paenglab, Assigned: Paenglab)

References

Details

Attachments

(1 file, 2 obsolete files)

With today build TB crashes when opening the in-content prefs. With normal pref all is working.
Problem are styles where we have caption > something, caption alone works. Our problem is, we use common.css from toolkit which has such selectors too. I don't know why FX works with this selectors and TB not.
(In reply to Richard Marti (:Paenglab) from comment #2)
> Problem are styles where we have caption > something, caption alone works.
> Our problem is, we use common.css from toolkit which has such selectors too.
> I don't know why FX works with this selectors and TB not.

Chances are that FX just doesn't have <xul:caption>s anymore. Given nesting captions is not common, probably removing the |>| combinator is just fine.

The other thing that can be done is what bug 1374247 did with <button>, which is avoiding <children>, but that assumes that captions are usually empty, which doesn't seem to be true looking at the codebase. It should also be possible to replace <caption><label>Foo</label></caption> with <caption label="Foo">, and then remove <children> though.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #3) 
> Chances are that FX just doesn't have <xul:caption>s anymore.

That makes sense. We still use the old XUL prefs to show in in-content.
Attached file in-contentPrefs.patch (obsolete) —
Changed all caption > * selectors to caption *. Unfortunately I had to copy all code from toolkit's common.css to aboutPreferences.css to remove the '>' selector. Removing the '>' makes also the specificity lesser, but I have no issue found.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8925292 - Flags: review?(jorgk)
Hmm, looks like I'm the wrong reviewer for this since I'm not at all familiar with these issues and I also don't quite understand what the problem is.

So common.css has, for example,

xul|button[type="menu"] > xul|*.button-box > xul|*.button-menu-dropmarker {
  -moz-appearance: none !important;
}

and you propose:
button[type="menu"] > .button-box > .button-menu-dropmarker {
  -moz-appearance: none !important;
}

Not quite a strict copy. Same goes for stuff copied from toolkit/themes/shared/in-content/common.inc.css.

Is there no chance that toolkit would remove those selectors? And why does CSS cause crashes?

Maybe Emilio can take the review.
Flags: needinfo?(emilio)
The xul| and html| are namespaces and common.css has them because it is used for XUL files and also normal HTML files. It's not the CSS that crashes but the CSS-processor. As I read bug 1374247 it's because stylo doesn't support child combinators like '>'.
(In reply to Richard Marti (:Paenglab) from comment #7)
> The xul| and html| are namespaces and common.css has them because it is used
> for XUL files and also normal HTML files. It's not the CSS that crashes but
> the CSS-processor. As I read bug 1374247 it's because stylo doesn't support
> child combinators like '>'.

It's not a matter of not supporting the '>' combinator. It's a matter of not support transparently matching them across <children> elements.

The reason this is crashing is to detect whether a hack we've removed causes different results in the styling of an element, so we can confidently remove the hack without regressing the UI in silent ways.

But under any case there's no reason this should require copy-pasting all the selectors, just removing the affected selectors or changing them to something sensible is fine.

And yeah, toolkit can definitely remove these selectors or change them. If they're not crashing already it means they're useless right now.
Flags: needinfo?(emilio)
Comment on attachment 8925292 [details]
in-contentPrefs.patch

Okay, trying to add a toolkit patch.
Attachment #8925292 - Attachment is patch: false
Attachment #8925292 - Flags: review?(jorgk)
Only changing the c-c selectors which makes the. The removal of the radio binding is a port of bug 1411640.
Attachment #8925292 - Attachment is obsolete: true
Attachment #8925297 - Flags: review?(jorgk)
Attached patch mozilla-central patch (obsolete) — Splinter Review
(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> And yeah, toolkit can definitely remove these selectors or change them. If
> they're not crashing already it means they're useless right now.

Johann, is it okay to change the selectors? Or do you like to remove them?
Attachment #8925298 - Flags: review?(jhofmann)
Comment on attachment 8925297 [details] [diff] [review]
comm-central patch

Review of attachment 8925297 [details] [diff] [review]:
-----------------------------------------------------------------

Somehow this patch looks a whole lot better than the previous one ;-)

::: mail/themes/windows/mail/preferences/aboutPreferences.css
@@ -15,5 @@
>  }
>  
> -@media not all and (-moz-windows-default-theme) {
> -  .paneButtonIcon {
> -    fill: ThreeDHighlight;

Why remove this?
Attachment #8925297 - Flags: review?(jorgk) → review+
(In reply to Jorg K (GMT+2) from comment #12)
> Comment on attachment 8925297 [details] [diff] [review]
> comm-central patch
> 
> Review of attachment 8925297 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: mail/themes/windows/mail/preferences/aboutPreferences.css
> @@ -15,5 @@
> >  }
> >  
> > -@media not all and (-moz-windows-default-theme) {
> > -  .paneButtonIcon {
> > -    fill: ThreeDHighlight;
> 
> Why remove this?

Ah yes, this was a remnant from pre Photon style. This made now on Win7 Classic light grey icons on the grey background.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> It's not a matter of not supporting the '>' combinator. It's a matter of not
> support transparently matching them across <children> elements.
> The reason this is crashing is to detect whether a hack we've removed causes
> different results in the styling of an element, so we can confidently remove
> the hack without regressing the UI in silent ways.
Hmm, but a website can't crash FF that way?
(In reply to Jorg K (GMT+2) from comment #14)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> > It's not a matter of not supporting the '>' combinator. It's a matter of not
> > support transparently matching them across <children> elements.
> > The reason this is crashing is to detect whether a hack we've removed causes
> > different results in the styling of an element, so we can confidently remove
> > the hack without regressing the UI in silent ways.
> Hmm, but a website can't crash FF that way?

No, this is XBL code only, and XBL is only available to privileged contexts and stuff like the video controls, which also aren't exposed to web content.
Comment on attachment 8925298 [details] [diff] [review]
mozilla-central patch

Review of attachment 8925298 [details] [diff] [review]:
-----------------------------------------------------------------

I don't really get why we can remove them, don't these selectors crash? That means they're used, no? In any case, I think this works for me.
Attachment #8925298 - Flags: review?(jhofmann) → review+
Aryx, please can you land the M-C patch?
Flags: needinfo?(aryx.bugmail)
Keywords: checkin-needed
Whiteboard: leave-open
Keywords: leave-open
Whiteboard: leave-open
Note that the mozilla-central patch is superseded by bug 1414786 I think.
Richard, sorry for the delay, does this still need to land?
Flags: needinfo?(aryx.bugmail) → needinfo?(richard.marti)
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #19)
> Richard, sorry for the delay, does this still need to land?

No, with bug 1414786 the same fix, and more, is already in autoland.
Flags: needinfo?(richard.marti)
Attachment #8925298 - Attachment is obsolete: true
Keywords: leave-open
Good, I'll land the C-C patch when bug 1414786 merges.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9c94f3d65e5a
Fix the in-content prefs after landing of the XBL compat hack (bug 1374247). r=jorgk CLOSED TREE
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 58.0
You need to log in before you can comment on or make changes to this bug.