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)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 28
People
(Reporter: mbrubeck, Assigned: jimm)
References
Details
(Whiteboard: [block28])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
mbrubeck
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•11 years ago
|
Assignee: nobody → jmathies
Assignee | ||
Comment 1•11 years ago
|
||
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.
Assignee | ||
Comment 2•11 years ago
|
||
The added complexity of the former solution might be useful in getting viewport stuff work in bug 801186.
Comment 3•11 years ago
|
||
I would also lean towards the latter solution here.
Assignee | ||
Comment 4•11 years ago
|
||
Assignee | ||
Comment 5•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #833050 -
Flags: review?(mbrubeck)
Assignee | ||
Comment 6•11 years ago
|
||
no longer relies on bug 937185. Also fixed a typo in the original.
Attachment #833050 -
Attachment is obsolete: true
Attachment #8333486 -
Flags: review?(mbrubeck)
Assignee | ||
Updated•11 years ago
|
Whiteboard: [triage] → [block28]
Reporter | ||
Comment 7•11 years ago
|
||
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+
Assignee | ||
Comment 8•11 years ago
|
||
(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.
Assignee | ||
Comment 9•11 years ago
|
||
Backed out alongside bug 932783 for being a possible cause for the bustage of mochitest-metro: https://hg.mozilla.org/integration/fx-team/rev/b43a066f1619
https://tbpl.mozilla.org/php/getParsedLog.php?id=30726849&tree=Fx-Team
Assignee | ||
Comment 11•11 years ago
|
||
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.
Assignee | ||
Comment 12•11 years ago
|
||
pushed update to try for an mc test run -
https://tbpl.mozilla.org/?tree=Try&rev=d41d3a615cf4
Assignee | ||
Comment 13•11 years ago
|
||
Comment 14•11 years ago
|
||
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.
Description
•