Closed Bug 937750 Opened 11 years ago Closed 11 years ago

Initial displayport for a Metro tab is too small

Categories

(Firefox for Metro Graveyard :: Pan and Zoom, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 28

People

(Reporter: mbrubeck, Assigned: jimm)

References

Details

(Whiteboard: [block28])

Attachments

(1 file, 1 obsolete file)

When a page first loads, or when a tab is selected, we set the displayport equal to the screen size:

http://hg.mozilla.org/mozilla-central/file/54e8c6492dc4/browser/metro/base/content/apzc.js#l41

Instead we should initialize the displayport to AsyncPanZoomController::CalculatePendingDisplayPort, like B2G does in TabChild::HandlePossibleViewportChange.
Assignee: nobody → jmathies
This has gotten interesting. My original idea was to fire an observer from the apzc.js code down to our APZController which would then use similar code to what you find in TabChild. However in order for that to work I needed the scroll/view id for the root of the tab. It turns out though that the scroll id isn't set by the time pageshow fires in apzc.js for the tab. If I wait for about ten msec after pageshow the display list gets built and the id is available. So I'm not sure yet when this should get called, maybe after the first paint of the tab or some other later event.

Alternatively we could keep the apzc call to setDisplayPortForElement we make now but maybe expand the values. Something simple like tab width/height * 1.5. This only needs to compensate for the very first scroll, after which the apzc should take over calculating the displayport.

Considering all the complexity of the former solution, I'm leaning toward the latter.
The added complexity of the former solution might be useful in getting viewport stuff work in bug 801186.
Depends on: 937185
I would also lean towards the latter solution here.
Attached patch fix (obsolete) — Splinter Review
Comment on attachment 833050 [details] [diff] [review]
fix

You'll need the patch in bug 937185 to apply this.

Note the display port is independent of scroll offset. So when scrolled (ex: scroll about:start, flip to another tab, flip back) the origin needs to be negative for the tab.
Attachment #833050 - Flags: review?(mbrubeck)
No longer depends on: 937185
Attachment #833050 - Flags: review?(mbrubeck)
Attached patch fixSplinter Review
no longer relies on bug 937185. Also fixed a typo in the original.
Attachment #833050 - Attachment is obsolete: true
Attachment #8333486 - Flags: review?(mbrubeck)
Whiteboard: [triage] → [block28]
Comment on attachment 8333486 [details] [diff] [review]
fix

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

::: browser/metro/base/content/apzc.js
@@ +46,4 @@
>            break;
>          }
> +      // intentional fall through
> +      case 'TabSelect':

Nit: Put {} around this "case" block (like the one below), so the "let" bindings can't leak out into other cases.

@@ +57,5 @@
> +        let portWidth = ContentAreaObserver.width;
> +        let portHeight = ContentAreaObserver.height;
> +
> +        if (portWidth < doc.scrollWidth) {
> +          portWidth += ContentAreaObserver.width * factor;

This could end up larger than the scrollWidth in some cases.  Is that a concern, or do we handle that gracefully?
Attachment #8333486 - Flags: review?(mbrubeck) → review+
(In reply to Matt Brubeck (:mbrubeck) from comment #7)
> Comment on attachment 8333486 [details] [diff] [review]
> fix
> 
> Review of attachment 8333486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/base/content/apzc.js
> @@ +46,4 @@
> >            break;
> >          }
> > +      // intentional fall through
> > +      case 'TabSelect':
> 
> Nit: Put {} around this "case" block (like the one below), so the "let"
> bindings can't leak out into other cases.
> 
> @@ +57,5 @@
> > +        let portWidth = ContentAreaObserver.width;
> > +        let portHeight = ContentAreaObserver.height;
> > +
> > +        if (portWidth < doc.scrollWidth) {
> > +          portWidth += ContentAreaObserver.width * factor;
> 
> This could end up larger than the scrollWidth in some cases.  Is that a
> concern, or do we handle that gracefully?

the dom appears to handle that gracefully - I didn't see any issues with tabs that had portWidths greater than the overall area. I should probably limit this all the same though to avoid future bugs.
Blocks: 900092
Obviously this was the patch - 

15:40:10  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/browser_context_ui.js | uncaught exception - TypeError: doc is null at chrome://browser/content/apzc.js:57

I guess if doc is null we should skip setting the display port all together.
pushed update to try for an mc test run - 

https://tbpl.mozilla.org/?tree=Try&rev=d41d3a615cf4
https://hg.mozilla.org/mozilla-central/rev/3c2b8af776cc
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: