Closed
Bug 579776
Opened 14 years ago
Closed 7 years ago
position:fixed and position:absolute shouldn't turn display:-moz-box into display:block
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: mstange, Unassigned)
References
Details
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/html
|
Details | |
(deleted),
application/octet-stream
|
Details |
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.
Attachment #458210 -
Flags: feedback?(bzbarsky)
Reporter | ||
Comment 1•14 years ago
|
||
(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?
Reporter | ||
Comment 2•14 years ago
|
||
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.
Reporter | ||
Comment 3•14 years ago
|
||
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?
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•14 years ago
|
||
(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+)
Reporter | ||
Comment 6•14 years ago
|
||
(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.
Reporter | ||
Comment 7•14 years ago
|
||
Patch without changes to IsBlockOutside(). Now the MathML check that EnsureBlockDisplay() mentions is out of sync - does that matter? If so, when?
Attachment #458210 -
Attachment is obsolete: true
Attachment #458363 -
Flags: review?(bzbarsky)
Attachment #458210 -
Flags: feedback?(bzbarsky)
Reporter | ||
Comment 8•14 years ago
|
||
(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•14 years ago
|
||
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•14 years ago
|
||
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. :(
Reporter | ||
Comment 11•14 years ago
|
||
(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•14 years ago
|
||
Yes, exactly.
Comment 13•14 years ago
|
||
(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.
Reporter | ||
Comment 14•14 years ago
|
||
(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.
Assignee: mstange → nobody
Status: ASSIGNED → NEW
Reporter | ||
Updated•14 years ago
|
Attachment #458363 -
Flags: review?(bzbarsky)
Comment 16•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
should only see green, no red visible
Updated•14 years ago
|
Summary: position:fixed shouldn't turn display:-moz-box into display:block → position:fixed and position:absolute shouldn't turn display:-moz-box into display:block
Comment 21•13 years ago
|
||
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•7 years ago
|
||
Let's call this WONTFIX per comment 10 and comment 14, and since we're moving away from using XUL anyway.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•