950180 - window.innerWidth is not up-to-date in window.onresize listener

Status: RESOLVED FIXED

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

Description

[Kartikaya Gupta (email:[e-mail])]

+++ This bug was initially created as a clone of Bug #940889 +++

1. Use Firefox OS browser to open the test page (see URL field of the bug) in portrait mode.
2. Use logcat to watch console output.
3. Rotate device to landscape mode.

Actual results:

window.innerWidth is not up-to-date in window.onresize listener as indicated by the log.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

This is the B2G equivalent of bug 940889 and the patch is very similar. Basically moving the GetPageSize call (which triggers the resize event) so that we set the scrollClamping size first.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #1)
> Created attachment 8347405 [details] [diff] [review]
> Patch
> 
> This is the B2G equivalent of bug 940889 and the patch is very similar.
> Basically moving the GetPageSize call (which triggers the resize event) so
> that we set the scrollClamping size first.

Don't you want to call SetCSSViewport after the call that set the scrollClamping ? My understanding was that the resize event was fired because of the call to SetCSSViewport that ends up calling presShell->ResizeReflowOverride and fire the resize event from http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#1975

Did I missed something ?

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8347405 [details] [diff] [review]
Patch

Review of attachment 8347405 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/TabChild.cpp
@@ +545,5 @@
> +  }
> +  // For non-HTML content (e.g. SVG), just assume page size == viewport size.
> +  return aViewport;
> +}
> +

Would this read better if you took care of the "nothing is there" case at the top, then worry about the rest of the code?

if (!htmlDOMElement && !bodyDOMElement) {
  return aViewport;
}
int32_t htmlWidth = ...

Comment 4

[Kartikaya Gupta (email:[e-mail])]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #2)
> Don't you want to call SetCSSViewport after the call that set the
> scrollClamping ? My understanding was that the resize event was fired
> because of the call to SetCSSViewport that ends up calling
> presShell->ResizeReflowOverride and fire the resize event from
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.
> cpp#1975
> 
> Did I missed something ?

As discussed on IRC: it's not actually the call to SetCSSViewport that triggers the resize event. The resize event is dispatched on the next reflow which happens lazily; in this case the call to GetPageSize forces a reflow because layout doesn't know what the new page size will be until it reflows. So as long as we set the scrollclamping size before doing anything that triggers a reflow we're fine, and that's what this patch does. It's not really the most intuitive thing unfortunately.

(In reply to Milan Sreckovic [:milan] from comment #3)
> Would this read better if you took care of the "nothing is there" case at
> the top, then worry about the rest of the code?
> 
> if (!htmlDOMElement && !bodyDOMElement) {
>   return aViewport;
> }
> int32_t htmlWidth = ...

Yup, I can do that.

Comment 5

[Chris Lord [:cwiiis]]

Comment on attachment 8347405 [details] [diff] [review]
Patch

Review of attachment 8347405 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

::: dom/ipc/TabChild.cpp
@@ +543,5 @@
> +    return CSSSize(std::max(htmlWidth, bodyWidth),
> +                   std::max(htmlHeight, bodyHeight));
> +  }
> +  // For non-HTML content (e.g. SVG), just assume page size == viewport size.
> +  return aViewport;

I agree with Milan's comment about this, but seeing as this is just a code move, perhaps we'd want to in a separate patch. Your call.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

I rearranged the code as per Milan's comment landed on b2g-i:

https://hg.mozilla.org/integration/b2g-inbound/rev/2688c4d13b43

Comment 7

[Kartikaya Gupta (email:[e-mail])]

I think this is worth uplifting since it is a regression exposed to browser content if nothing else.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2688c4d13b43

Comment 9

[Andrew Overholt [:overholt]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> I think this is worth uplifting since it is a regression exposed to browser
> content if nothing else.

WFM

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2217681c1b68