Closed
Bug 283686
Opened 20 years ago
Closed 16 years ago
root element can not have display:none
Categories
(Core :: Layout, defect, P2)
Core
Layout
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)
72 bytes,
text/html
|
Details | |
6.61 KB,
patch
|
Details | Diff | Splinter Review | |
177 bytes,
text/html
|
Details | |
5.38 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
For a testcase you can view attachment 133482 [details]. I will also attach a simple
text/html testcase.
Reporter | ||
Comment 1•20 years ago
|
||
![]() |
||
Comment 2•20 years ago
|
||
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....
Comment 3•19 years ago
|
||
*** Bug 313010 has been marked as a duplicate of this bug. ***
Updated•17 years ago
|
Whiteboard: [CSS1-5.6.1]
Comment 4•16 years ago
|
||
Assignee | ||
Comment 5•16 years ago
|
||
Also see the changes made in bug 454361.
Comment 6•16 years ago
|
||
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.
Assignee | ||
Comment 7•16 years ago
|
||
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?
Comment 8•16 years ago
|
||
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
Assignee | ||
Updated•16 years ago
|
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?
Assignee | ||
Comment 10•16 years ago
|
||
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).
Assignee | ||
Comment 11•16 years ago
|
||
(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.
Assignee | ||
Comment 14•16 years ago
|
||
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.
Assignee | ||
Comment 15•16 years ago
|
||
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)
![]() |
||
Updated•16 years ago
|
Attachment #356096 -
Flags: superreview?(bzbarsky)
Attachment #356096 -
Flags: superreview+
Attachment #356096 -
Flags: review?(bzbarsky)
Attachment #356096 -
Flags: review+
Assignee | ||
Updated•16 years ago
|
Whiteboard: [CSS1-5.6.1][has wip patch] → [CSS1-5.6.1][needs landing]
Assignee | ||
Comment 16•16 years ago
|
||
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: 16 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
Comment 17•16 years ago
|
||
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.
Assignee | ||
Comment 18•16 years ago
|
||
Sounds exactly like bug 472980, which is separate from this bug.
Assignee | ||
Comment 19•16 years ago
|
||
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
Comment 20•16 years ago
|
||
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+
Keywords: fixed1.9.1 → verified1.9.1
Target Milestone: mozilla1.9.1b3 → mozilla1.9.2a1
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
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.
Description
•