283686 - root element can not have display:none

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Anne (:annevk)]

For a testcase you can view attachment 133482 [details]. I will also attach a simple
text/html testcase.

Comment 1

[Anne (:annevk)]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Partial patch

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

[Erik Fabert]

*** Bug 313010 has been marked as a duplicate of this bug. ***

Comment 4

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also see the changes made in bug 454361.

Comment 6

[Mats Palmgren (inactive)]

Attachment: wip

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.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 8

[Mats Palmgren (inactive)]

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.

Comment 9

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

(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?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(by runtime check, I mean changing what's now an assertion to be a real test)

Comment 12

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

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.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'll take it.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for dynamic changes

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.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 17

[Martin Packer]

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.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Sounds exactly like bug 472980, which is separate from this bug.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7b5240017a6d

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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