579776 - position:fixed and position:absolute shouldn't turn display:-moz-box into display:block

Status: RESOLVED WONTFIX

(Core :: Layout, defect)

Description

[Markus Stange [:mstange]]

Attachment: keep display:-moz-box on fixed/absolutely positioned elements

When you right-click on a tab and select "Make into App Tab", position:fixed is set on the tab. That enables us to put it at the left edge of the tab bar and to take it out of the tab overflow scrollbox without having to move the node in the DOM.

However, in bug 547787 and bug 579632 we noticed that this creates some problems: children of the tab suddenly end up in different positions than without position:fixed.
It turns out that this is because position:fixed makes the element behave as if display:block was set on it. So for example -moz-box-align stops working and the tab's children are subject to line-wrapping instead of XUL's usual squeeze-to-fit behavior.

Can we just stop doing that?

The patch I've attached stops EnsureBlockDisplay from changing display:-moz-box. However, a comment there and an assertion in nsCSSFrameConstructor::FindDisplayData say that nsStyleDisplay::IsBlockOutside needs to be kept in sync with this. So I've made IsBlockOutside return true for display:-moz-box. But this in turn changes behavior: elements with display:-moz-box inside a display:block element will now behave like block elements instead of like inline elements.

Is that a change we can make? I don't think having display:-moz-box inside display:block is very common. I had to change two reftests from display:-moz-box to display:-moz-inline-box because they relied on those boxes being inline.

I've noticed one thing in the Firefox UI that was broken by this change: In the new add-ons manager, the five star images of an extension's rating are now under each other instead of next to each other. They're XUL images which are *inside* a XUL label - that was probably not intentional.

I also had to remove FCDATA_DISALLOW_OUT_OF_FLOW from XUL elements - without that change, position:fixed suddenly behaved like position:static.

I've pushed this patch to the tryserver. The patch also needs a test.

Comment 1

[Markus Stange [:mstange]]

(In reply to comment #0)
> But this in turn changes behavior: elements with
> display:-moz-box inside a display:block element will now behave like block
> elements instead of like inline elements.

This also applies to XUL elements inside XUL <label>, <description> and <text> elements. Is it common to have that? The accessibility test test_name.xul actually tests it:

234   <!-- the name from subtree, mixed content -->
235   <description id="labelledby_mixed">
236     no<description>more text</description>
237   </description>

... and starts failing with this patch because the description is now on a separate line. Alex, what was the reason you added this test? Was it inspired by a real world usage of that pattern?

Comment 2

[Markus Stange [:mstange]]

There's another change to the patch I need to make, because at the moment nested display:-moz-box; position:fixed elements will lead to a crash:

<box id="outer" style="position:fixed">
  <box id="inner style="position:fixed"/>
</box>

When laying out #inner, nsHTMLReflowState::GetHypotheticalBoxContainer looks for the nearest containing block of #inner's placeholder frame. But #outer is not a containing block, so the ViewportFrame will be used as the containing block. However, since cbrs->frame is also the ViewportFrame, this will lead to a crash in a do...while loop in nsHTMLReflowState::CalculateHypotheticalBox which assumes that cbrs->frame is a real ancestor of aContainingBlock.

Nested display: *block*; position:fixed elements don't have this problem: Then #outer will be the containing block and everything's fine.

So in other words, nested position:fixed works as long as all display types that support position:fixed are also containing blocks. Can I just make display:-moz-box frames be containing blocks, too? What implications does this have?

I'll kick off another tryserver run with a changed nsFrame::IsContainingBlock() implementation that also returns true for NS_STYLE_DISPLAY_BOX.

Comment 3

[Markus Stange [:mstange]]

I guess another option that would avoid changing box-in-block behavior would be to add a new method, CanBeBlockOutside, that returns true for -moz-box and is used in the assertion. Then we don't have to change IsBlockOutside.
Should I do that instead?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think just changing code to fix the assertion in FindDisplayData without changing other behavior like IsBlockOutside is preferable.

However, that means you still have to fix comment #2. Making nsFrame::IsContainingBlock return true for non-inline flexboxes makes sense to me.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #1)
> 234   <!-- the name from subtree, mixed content -->
> 235   <description id="labelledby_mixed">
> 236     no<description>more text</description>
> 237   </description>
> 
> ... and starts failing with this patch because the description is now on a
> separate line. Alex, what was the reason you added this test? Was it inspired
> by a real world usage of that pattern?

Alexander will have limited connectivity for a couple of weeks. I think you are fine to change the a11y test to:

<description id="labelledby_mixed">
  no<span>more text</span>
</description>

(Assuming it passes of course, I can r+)

Comment 6

[Markus Stange [:mstange]]

(In reply to comment #4)
> I think just changing code to fix the assertion in FindDisplayData without
> changing other behavior like IsBlockOutside is preferable.

OK. Looks like I don't even need to fix the assertion - if display is -moz-box, I'll end up in FindXULDisplayData and never arrive in FindDisplayData.

Comment 7

[Markus Stange [:mstange]]

Attachment: v2

Patch without changes to IsBlockOutside(). Now the MathML check that EnsureBlockDisplay() mentions is out of sync - does that matter? If so, when?

Comment 8

[Markus Stange [:mstange]]

(In reply to comment #5)
> Alexander will have limited connectivity for a couple of weeks. I think you are
> fine to change the a11y test to:
> 
> <description id="labelledby_mixed">
>   no<span>more text</span>
> </description>
> 
> (Assuming it passes of course, I can r+)

Thanks, David, but it turned out that we can just keep the existing behavior here, so I don't need to change the test after all.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oy.  So in order of stuff from comment 0:

1)  We _could_ stop changing the display for out-of-flow moz-box.  In fact, it
    makes sense to.  We probably need to audit places that deal with
    out-of-flows to make sure they can deal with it.
2)  It's probably fine to not change IsBlockOutside().
3)  FCDATA_DISALLOW_OUT_OF_FLOW is there because XUL is totally not set up to
    really handle out-of-flows.  You already discovered this with the one crash
    you ran into; there are probably others lurking.  I would expect absolute
    positioning to be even worse than fixed positioning; no idea about floats. 
    A significant amount of testing (please talk to Jesse about this!) and code
    auditing would probably need to happen before I would be happy approving
    that change.  I'd certainly want to see a list of what all code was looked
    at (and reviewing that will be ... fun).

I'm not quite sure what all the implications of the proposed IsContainingBlock change would be.

I'd also be interested in any insight dbaron has on how CSS plans to specify this stuff before thrashing too much here.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, and my general take on this is that changing invariants like this is _really_ scary.  Especially where the mess that is XUL code and the broken undocumented assumptions it tends to make is involved.  :(

Comment 11

[Markus Stange [:mstange]]

(In reply to comment #9)
> I'd certainly want to see a list of what all code was looked
> at (and reviewing that will be ... fun).

Almost nothing - I just tried it out and was surprised at how smoothly it went through our tests. But that probably indicates lack of test coverage rather than lack of bugs.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Yes, exactly.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> (In reply to comment #0)
> > But this in turn changes behavior: elements with
> > display:-moz-box inside a display:block element will now behave like block
> > elements instead of like inline elements.
> 
> This also applies to XUL elements inside XUL <label>, <description> and <text>
> elements. Is it common to have that? The accessibility test test_name.xul
> actually tests it:
> 
> 234   <!-- the name from subtree, mixed content -->
> 235   <description id="labelledby_mixed">
> 236     no<description>more text</description>
> 237   </description>
> 
> ... and starts failing with this patch because the description is now on a
> separate line. Alex, what was the reason you added this test? Was it inspired
> by a real world usage of that pattern?

More likely it was tests for name calculation logic rather than real case.


(In reply to comment #8)
> (In reply to comment #5)
> > <description id="labelledby_mixed">
> >   no<span>more text</span>
> > </description>

technically I would like to see fixed accessible name rather than markup but any way since

> > (Assuming it passes of course, I can r+)
> 
> Thanks, David, but it turned out that we can just keep the existing behavior
> here, so I don't need to change the test after all.

then it's perfect, thanks.

Comment 14

[Markus Stange [:mstange]]

(In reply to comment #9)

Actually, all this sounds like too much unnecessary work. It's not like display:block is constraining us in any way.

Unassigning.

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

So, according to the dupe, this bug is also about our HTML5 support for display:box (-moz-box) which is currently broken on any position:absolute|fixed elements.

An example is a simple slide-like master template: http://jsfiddle.net/5MZbV/

Does it elevate this bug any higher? :)

Comment 17

[Boris Zbarsky [:bzbarsky]]

What "html5 support"?  There's nothing html5-specific here.

What would help fixing this bug is not adding noise, and perhaps writing comprehensive testcases for the corner cases I mention in comment 9....

Comment 18

[caveman]

Attachment: test case

should only see green, no red visible

Comment 21

[John A. Bilicki III]

Attachment: Demonstration of work-around for this bug, cross-browser compatible (Firefox 3.0+, Safari 3.0+, IE10+).

Not sure if this bug has been forgotten though I was luckily able to find a work-around (albeit a messy one) where the rules can be applied to a child element of an absolutely positioned element and Gecko will render it as desired. The problem is that it requires an extra element however this test case produces the same results in Firefox 3, 10, Safari 3.0, 5.1, Chrome 18 and Internet Explorer 10.

Comment 22

[Daniel Holbert [:dholbert]]

Let's call this WONTFIX per comment 10 and comment 14, and since we're moving away from using XUL anyway.