377539 - nsIDOMScreen.width exception with display:none in parent frame

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Robert Accettura [:raccettura]]

When getting the screen.width from within an iframe that is display:none, nsIDOMScreenwidth fails.  

Considering if display:none weren't in place it would return the size of the parent... I think it should do so regardless of the style.

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMScreen.width]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame ::

Testcase coming.

Comment 1

[Robert Accettura [:raccettura]]

Attachment: Testcase (Frame)

Comment 2

[Robert Accettura [:raccettura]]

Attachment: Testcase (Page)

Comment 3

[Robert Accettura [:raccettura]]

Clicking on the word "test" in attachment 261582 [details] shall serve as a testcase.

Comment 4

[Robert Accettura [:raccettura]]

Testing this in IE shows that my assumption of it still returning the parent size was correct.

Comment 5

[Mats Palmgren (inactive)]

The testcase works in Opera 9.02 too.

Comment 6

[Mats Palmgren (inactive)]

Attachment: wip (diff -w)

The pres context is null.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/dom/src/base/nsScreen.cpp&rev=1.28&root=/cvsroot&mark=199,227#199

Maybe we should look for one in an ancestor docshell?

Comment 7

[Boris Zbarsky [:bzbarsky]]

What happens in a multi-monitor setup?  Note that there, the screen for a subframe may not be the same as the screen for the parent...

That said, if we have no display, using the screen of the parent makes sense.  So in general this seems reasonable.

Comment 8

[Mats Palmgren (inactive)]

(In reply to comment #7)
> What happens in a multi-monitor setup?  Note that there, the screen for a
> subframe may not be the same as the screen for the parent...

Ok, but it can't be completely window-less (native) and still be
associated with a different screen can it?
I can run a test if you can suggest how I would create the case you're
looking for.

Comment 9

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1 (diff -w)

An alternative solution would be to get the device context through
  docshell.QI(nsIBaseWindow)->GetMainWidget()->GetDeviceContext()
instead of what we do now
  docshell->GetContentViewer()->GetPresContext()->DeviceContext()

With the alternative solution we find the device context on the first
docshell for the testcase.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Ok, but it can't be completely window-less (native) and still be
> associated with a different screen can it?

Right.  That's what the second half of comment 7 was about...

Comment 11

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 261629 [details] [diff] [review]
Patch rev. 1 (diff -w)

Yeah, looks reasonable.

Comment 13

[Mats Palmgren (inactive)]

Checked in to trunk at 2007-05-08 02:44 PDT.

-> FIXED

Comment 14

[Mats Palmgren (inactive)]

Attachment: Mochitest, rev. 1

Add mochitest dom/tests/mochitest/bugs/test_bug377539-1.html

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 264093 [details] [diff] [review]
Mochitest, rev. 1

>Index: dom/tests/mochitest/bugs/Makefile.in
>+		test_bug377539-1.html \

Do you expect more mochitests for this bug?  If not, why bother with the "-1"?

>+<div id="content" style="display: none">

Why not just put the iframe in here instead of the dynamic stuff you do?  I think it would be clearer...

The rest looks good.

Comment 16

[Mats Palmgren (inactive)]

I fixed the filename and checked in the mochitest at 2007-05-09 06:27 PDT.
I tried to remove the dynamic stuff but it didn't work, I don't know why,
my JS sucks, feel free to fix it ;-)