Closed Bug 283686 Opened 15 years ago Closed 11 years ago

root element can not have display:none

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: annevk, Assigned: dbaron)

References

Details

(Keywords: css1, testcase, verified1.9.1, Whiteboard: [CSS1-5.6.1])

Attachments

(4 files, 1 obsolete file)

For a testcase you can view attachment 133482 [details]. I will also attach a simple
text/html testcase.
Attached file testcase
Attached patch Partial patchSplinter Review
This asserts (because the undisplayed content has no parent).  More
importantly, this breaks focusing the content area (using the testcase in this
bug) -- I can neither tab to the content area, nor focus it via click.	That
seems like a problem....
*** Bug 313010 has been marked as a duplicate of this bug. ***
Whiteboard: [CSS1-5.6.1]
Attached file Testcase #2
Also see the changes made in bug 454361.
Attached patch wip (obsolete) — Splinter Review
Is there a reason we don't want to hash null in nsFrameManager?
This seems to fix Testcase #2, although not the focus problem Boris noted.
Nominating this for blocking1.9.1 because, given the changes we made in bug 454361, this can now cause the root element to become forever-undisplayed (instead of forever-displayed).  In other words, we now support making the root element display:none, but once it's there, you can't get it back.  That seems like a bad state to be in, and I could even imagine it breaking sites if people are using toggling display on the root as a workaround for another browser.
Flags: blocking1.9.1?
I think the patch is fine if it were not for bug 463511 where we have
a call with content from within an anonymous subtree (GetParent() is 
null in that case too).

The focus problem is a separate bug (pres shell event delivery), I have
a fix for that locally.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Whiteboard: [CSS1-5.6.1] → [CSS1-5.6.1][has wip patch]
(In reply to comment #8)
> I think the patch is fine if it were not for bug 463511 where we have
> a call with content from within an anonymous subtree (GetParent() is 
> null in that case too).

I don't understand. Does this patch make that bug worse?
Maybe in SetUndisplayedContent we should just do a runtime check for if it's the root content?  That seems like it could also be useful in the future depending on how we implement XBL2 (or even how svg:use is implemented).
(by runtime check, I mean changing what's now an assertion to be a real test)
Mats, Boris, David, can one of you own this? I'd work on it except I don't understand what the XBL issue is here.
I'll take it.
Assignee: nobody → dbaron
So, actually, I don't think anything goes wrong even with bug 463511.  The undisplayed map tolerates multiple children of the same content node being display:none; now it'll just be as though there are multiple children of "null" being display:none.  I don't see anything wrong with that.

I think we should change the assertion to fail if mPresShell or mPresShell->GetDocument() are null (rather than succeed and potentially cover a problem), though.

I'll post the patch with that change.
This is attachment 345755 [details] [diff] [review], the assertion change, and three reftests (static, dynamic from none, and dynamic to none).  This patch itself fixes the second reftest.
Attachment #345755 - Attachment is obsolete: true
Attachment #356096 - Flags: superreview?(bzbarsky)
Attachment #356096 - Flags: review?(bzbarsky)
Attachment #356096 - Flags: superreview?(bzbarsky)
Attachment #356096 - Flags: superreview+
Attachment #356096 - Flags: review?(bzbarsky)
Attachment #356096 - Flags: review+
Whiteboard: [CSS1-5.6.1][has wip patch] → [CSS1-5.6.1][needs landing]
Fixed: http://hg.mozilla.org/mozilla-central/rev/7bf1d1b7f3b8

I split off the focus problem into bug 472980, and I'm marking this bug as fixed.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [CSS1-5.6.1][needs landing] → [CSS1-5.6.1][needs 1.9.1 landing]
Target Milestone: --- → mozilla1.9.2a1
Interestingly - to me at least - the bug stops Ctrl+U from working. Have to go View->Page Source via the menus. (At least I'm hoping it IS this bug that does that.) Is this a consequence you'd expect of this bug? It sounds to me like it is.
Sounds exactly like bug 472980, which is separate from this bug.
Depends on: 473678
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7b5240017a6d
Keywords: fixed1.9.1
Whiteboard: [CSS1-5.6.1][needs 1.9.1 landing] → [CSS1-5.6.1]
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b3
Verified on trunk and 1.9.1 with builds on OS X and Windows:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre ID:20090204020327

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090203 Minefield/3.2a1pre
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Target Milestone: mozilla1.9.1b3 → mozilla1.9.2a1
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.