852420 - Suppress NeededToWrapXUL warning for generated content

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Matthew N. [:MattN]]

While working on the new Firefox tab shape, there was a regular spew of the following warning: "XUL box for _moz_generated_content_after element contained an inline #text child, forcing all its children to be wrapped in a block."

After talking with Boris, my understanding is that these can be suppressed for generated content. (see below)

#developers 2013-02-01:
[12:55:36] <mconley> bz: hey - so, I'm getting this warning in the error 
                     console: "Warning: XUL box for _moz_generated_content_after 
                     element contained an inline #text child, forcing all its 
                     children to be wrapped in a block." using ::after with 
                     content: ''…how does one avoid this?
[12:56:49] <bz> mconley: You don't?
[12:57:01] <bz> mconley: I assume you're using display:-moz-box on the :after or 
                something?
[12:57:09] <mconley> that's correct.
[12:57:21] <bz> mconley: The warning is just saying there's a potential perf 
                issue…
[12:57:29] <bz> mconley: I suppose we could suppress them for generated content…
[13:04:32] <bz> What happens if you set the ::after to be an inline-block?
[13:04:42] <bz> Does it just not stretch vertically to the right height?
[13:05:50] <mconley> bz: yes, it's a XUL hbox. Setting it as inline-block gets 
                     rid of the warning, but causes some stuff to shift around. 
                     Ok, we're going to investigate that route a little deeper 
                     though. Thanks!

After the discussion we went with inline-block but Dão suggested in bug 738491 comment 145 that mixing inline-block with -moz-box is not wise. You can take a look at the pseudo-elements using inline-block at https://bugzilla.mozilla.org/attachment.cgi?id=722702&action=diff#a/browser/themes/shared/tabs.inc.css_sec2
Could we get these warnings suppressed if they aren't relevant? Thanks.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, so the reason I asked dbaron for feedback is that the warnings _are_ relevant: using an inline here is slower and uses more memory than using an inline-block.  The question is whether for generated content in particular it's worth making an exception to the warning for some reason.

Comment 2

[David Baron :dbaron:]

I guess I'm ok suppressing it, though it's sort of "incorrect XUL" based on the original design of XUL as I understand it.  (Who owns XUL these days?)

Comment 3

[:Gijs (he/him)]

Attachment: Patch

Comment 4

[:Gijs (he/him)]

Attachment: diff -w

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 741258 [details] [diff] [review]
Patch

r=me

Comment 6

[Matthew N. [:MattN]]

Perhaps we could experiment with border-image for the separators instead now that we've switched to using an image for that since filing this bug. (Gradients are not supported in border-image in Gecko yet [bug 709587]). That is easier to maintain compared to the pseudo-elements but may cause new problems with tab overflow. 

I don't want to hide a warning that is relevant but I also don't really understand what exactly we're doing that is incorrect XUL. If it is just the |content| property that is causing an inline text child, then that could also change to url("chrome://browser/skin/tabbrowser/tab-separator.png") now. Gijs, can you take a quick look at these two possibilities before landing this?

Gijs, FYI: if we go ahead with this patch it should land on inbound instead of UX as it's not something we need to hold back for Australis.

Comment 7

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #6)
> Perhaps we could experiment with border-image for the separators instead now
> that we've switched to using an image for that since filing this bug.
> (Gradients are not supported in border-image in Gecko yet [bug 709587]).
> That is easier to maintain compared to the pseudo-elements but may cause new
> problems with tab overflow.

This seems to be feasible, although this means there's "unclickable" space inbetween the tabs, which is undesirable. I haven't looked at how to fix that, first I'd like to see how win32 and linux look with what I've concocted. 
 
> I don't want to hide a warning that is relevant but I also don't really
> understand what exactly we're doing that is incorrect XUL. If it is just the
> |content| property that is causing an inline text child, then that could
> also change to url("chrome://browser/skin/tabbrowser/tab-separator.png")
> now. Gijs, can you take a quick look at these two possibilities before
> landing this?

I can try this as well, it may be easier...

> Gijs, FYI: if we go ahead with this patch it should land on inbound instead
> of UX as it's not something we need to hold back for Australis.

The thing is, we also use (both!) ::before and ::after for (both of!) the sides of selected/hovered tabs themselves. So you still get 4 warnings for each tab you open even if we eliminate the separator ones.

Comment 8

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #7)
> (In reply to Matthew N. [:MattN] from comment #6)
> > Gijs, FYI: if we go ahead with this patch it should land on inbound instead
> > of UX as it's not something we need to hold back for Australis.
> 
> The thing is, we also use (both!) ::before and ::after for (both of!) the
> sides of selected/hovered tabs themselves. So you still get 4 warnings for
> each tab you open even if we eliminate the separator ones.

I've fixed the separators and the background-image case (bug 865698). The one with a gradient background we're better off not fixing (could do url() with a transparent data URI, but discussion on IRC seems to indicate that's unlikely to be more performant than the current solution). So I've still committed this to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/34819ac4912e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/34819ac4912e