Closed Bug 511224 Opened 10 years ago Closed 10 years ago

after zoom, titlebar is partially off screen

Categories

(Firefox for Android Graveyard :: Panning/Zooming, defect, critical)

defect
Not set
critical

Tracking

(fennec1.0+)

VERIFIED FIXED
fennec1.0b5
Tracking Status
fennec 1.0+ ---

People

(Reporter: madhava, Assigned: stechz)

References

Details

Attachments

(2 files, 6 obsolete files)

Attached image screenshot from n810
See the attached image.  This happens sometimes, but not all the time, after zooming with a nightly build from 2009-08-18.
I guess it happens each time you zoom with the sidebars hidden.
tracking-fennec: ? → 1.0+
Duplicate of this bug: 512009
I saw this on test day too on the n810... see Bug 512009 for reproduction
this is the case on windows mobile as well (using a nightly from trunk)
OS: Mac OS X → All
Hardware: x86 → All
This is still happening on build:

Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.2a2pre) Gecko/20090915 Fennec/1.0a3
Flags: in-litmus?
Assignee: nobody → webapps
This is an ugly UX bug that needs to be fixed. I'm marking this as critical ...and I'm glad it's been marked as blocking-fennec already.
Severity: normal → critical
Attached patch Add a third scrollbox (obsolete) — Splinter Review
Since the urlbar was in the same scrollbox as the content, when content is zoomed in, the width of the pane grew as well, making more area for the urlbar to cover.  The only real solution we thought of: another icky scrollbox.

While I was modifying the DOM, I went ahead and refactored a few things:
* There is new Point class for returning and passing x,y pairs.
* wsRect is gone, long live Util.Rect.
* Util.Rects can be empty sets, so that intersect does not have to return null and restrictTo doesn't have to throw things.  The intersect/union algorithms were also cleaned up.
* Generated CSS rules are dynamically updated for things that are calculated at runtime, like the urlbar height and the window height.  The resize code has been cleaned up.  I tried to only use these special classes where necessary (why can't I use width:100% on fixed position elements?).

All of these changes are dangerous, so we'll need to do some heavy testing on this patch on each platform (esp. Windows Mobile).  Also, I haven't tested the impact this patch has on perf.
Attachment #403889 - Flags: review?(pavlov)
That's cool to see the urlbar nicely after a zoom!

I have played a few with it and noticed to small things:
 * When I start fennec the UI seems to be shifted 1px to the right (I can see a 1px border of the left sidebar)

 * The background of the new scrollbox seems to be different than the color we use for sidebar - that's not so much but I think this can help for user perception of the flickering which happens when we quickly swith between unlocked/locked mode of the toolbar

Seems nice on Desktop, I'll try to test it on my n810 tomorrow
Attached patch vingetun tweaks (obsolete) — Splinter Review
I added panel-dark class to the vboxes containing the sidebars to help with white background on jerkiness and used Math.round in hideSidebars so that the correct position is scrolled to (scrollBy ignores anything fractional).  I also now try to float the toolbar before panning in hopes that it will help the urlbar "jumping" issue.
Attachment #403889 - Attachment is obsolete: true
Attachment #404130 - Flags: review?(pavlov)
Attachment #403889 - Flags: review?(pavlov)
Sorry for spam, reuploaded original attachment accidentally.
Attachment #404130 - Attachment is obsolete: true
Attachment #404131 - Flags: review?(pavlov)
Attachment #404130 - Flags: review?(pavlov)
Attachment #404131 - Flags: review?(mark.finkle)
Comment on attachment 404131 [details] [diff] [review]
Correct patch for vingetun tweaks

>   createBrowserViewportState: function createBrowserViewportState() {
>-    return new BrowserView.BrowserViewportState(new wsRect(0, 0, 1, 1), 0, 0, 1);
>+    return new BrowserView.BrowserViewportState(new Util.Rect(0, 0, 1, 1), 0, 0, 1);

Might be better to separate a large patch like wsRect -> Util.Rect

>     if (handheldFriendly == "true") {
>-      browser.handheld = true;
>-      browser.setAttribute("style", "width: " + window.screen.width + "px;");
>+      browser.className = "browser-handheld";
>       this.setZoomLevel(1);
>       browser.markupDocumentViewer.textZoom = 1;
>     } else {
>-      delete  browser.handheld;
>+      browser.className = "browser";

Why are you changing to className?

>     if (cr) {
>-      let [ctrx, ctry] = cr.centerRounded();
>+      let ctr = cr.centerRounded();
>       //dump('-----> centering at ' + ctrx + ',' + ctry + '\n');
>-      this.recenterEvictionQueue(ctrx, ctry);
>+      this.recenterEvictionQueue(ctr.x, ctr.y);


Separate patch would be better for the Util.Point class too and why are we still passing x, y into recenterEvictionQueue? Why not make it take a Point?

>-// -----------------------------------------------------------
>-// Util.Rect is a simple data structure for representation of a rectangle supporting
>-// many basic geometric operations.
>-//
>+/**
>+ * Simple Point class.
>+ *
>+ * Any method that takes an x and y may also take a point.
>+ */
>+Util.Point = function Point(x, y) {
>+  this.set(x, y);
>+}
>+
>+Util.Point.prototype = {
>+  clone: function clone() {

Is this needed?

>+  round: function round() {

>+  ceil: function ceil() {

>+  floor: function floor() {

These 3 functions seem like overkill to me. Maybe you could just add a function (or maybe .map(...) works?) to take a function and call it with this.x and this.y

>+  add: function add(x, y) {

>+  subtract: function subtract(x, y) {

Why do we need add and subtract? Wouldn't translate be good enough?

>+(function() {
>+  function takePointOrArgs(f) {
>+    return function(arg1, arg2) {
>+      if (arg2 === undefined)
>+        return f.call(this, arg1.x, arg1.y);
>+      else
>+        return f.call(this, arg1, arg2);
>+    };
>+  }
>+
>+  for each (let f in ['add', 'subtract', 'equals', 'set'])
>+    Util.Point.prototype[f] = takePointOrArgs(Util.Point.prototype[f]);
>+})();

Nice, but bordering on overkill

>+Util.Rect.fromRect = function fromRect(r) {
>+  return new Util.Rect(r.left, r.top, r.right - r.left, r.bottom - r.top);
>+}

>   clone: function clone() {
>-    return new wsRect(this.left, this.top, this.right - this.left, this.bottom - this.top);
>+    return new Util.Rect(this.left, this.top, this.right - this.left, this.bottom - this.top);
>   },

I keep asking... do we need clone?

>   centerRounded: function centerRounded() {
>-    return this.center().map(Math.round);
>+    return this.center().round();
>   },

I guess I don't like all these specialized methods, when the longer version is not much longer and is more explicit. E.g. :

  myRect.center().map(Math.round);

>diff -r 2725ad0332ff -r 450086ed62d9 chrome/content/browser.js

>+      Browser.styles['window-width'].width = w + "px";
>+      Browser.styles['toolbar-height'].height = toolbarHeight + "px";
>+      Browser.styles['browser'].width = kDefaultBrowserWidth + "px";
>+      Browser.styles['browser'].height = scaledDefaultH + "px";
>+      Browser.styles['browser-handheld'].width = window.screen.width + "px";
>+      Browser.styles['browser-handheld'].height = scaledScreenH + "px";

OK, now I see why you were using className above.

This patch does way too many things. You have at least 4 patches here:
* Remove wsRect
* Add Util.Point
* Use CSS styles for resizing
* After zoom titlebar is off screen

Let's convert these to separate patches please.
Attachment #404131 - Flags: review?(mark.finkle) → review-
I proposed changing wsRect to Util.Rect a few months ago, but Stuart shot it down because of the increased symbol lookup time by putting Rect into a namespace.
> I proposed changing wsRect to Util.Rect a few months ago, but Stuart shot it
down because of the increased symbol lookup time by putting Rect into a
namespace.

wsRect, Util.Rect, whatever.  But we should clearly not have two Rect classes.  And wsRect is a silly silly name.  Is there a reason Rect won't work?
wsRect comes from when it was originally done as part of the now-gone window stack code.  Let's keep Util.Rect and add an alias "var Rect = Util.Rect;"  I would hope that Rect wouldn't conflict with add-on code.  But, anyway, this should be a separate bug.
Blocks: 520872
Depends on: 520910
Depends on: 520913
Attached patch Code review changes (obsolete) — Splinter Review
The patch has been broken down into 3 separate patches.  Bug now depends on the CSS class and the Util refactoring.
Attachment #404131 - Attachment is obsolete: true
Attachment #404964 - Flags: review?(pavlov)
Attachment #404131 - Flags: review?(pavlov)
Attachment #404964 - Flags: review?(pavlov) → review?(mark.finkle)
Attached patch Sync with dependent patches (obsolete) — Splinter Review
Attachment #404964 - Attachment is obsolete: true
Attachment #406261 - Flags: review?(pavlov)
Attachment #404964 - Flags: review?(mark.finkle)
Attached patch Bit rot (obsolete) — Splinter Review
Attachment #406261 - Attachment is obsolete: true
Attachment #406576 - Flags: review?(gavin.sharp)
Attachment #406261 - Flags: review?(pavlov)
Attachment #406576 - Flags: review?(gavin.sharp) → review?(21)
Comment on attachment 406576 [details] [diff] [review]
Bit rot

>-    this.controlsScrollboxScroller.scrollBy(dx, 0);
>-    Browser.contentScrollbox.customDragger.scrollingOuterX = false; // XXX ugh.
>+    let container = document.getElementById("tile-container-container");
>+    let rect = container.getBoundingClientRect();
>+    this.controlsScrollboxScroller.scrollBy(Math.round(rect.left), 0);
>     this._browserView.onAfterVisibleMove();
>   },

use this.contentScrollbox instead of container?


>+  /** Return offset that pans controls away from screen. Updates doffset with leftovers. */
>+  _panControlsAwayOffset: function(doffset) {
>+    let x = 0, y = 0, scrollbox, rect;
>+
>+    scrollbox = Browser.pageScrollbox;
>+    rect = Rect.fromRect(scrollbox.getBoundingClientRect()).map(Math.round);
>+    if (doffset.x < 0 && rect.right < window.innerWidth)
>+      x = Math.max(doffset.x, rect.right - window.innerWidth);
>+    if (doffset.x > 0 && rect.left > 0)
>+      x = Math.min(doffset.x, rect.left);
>+
>+    scrollbox = Browser.contentScrollbox;
>+    rect = Rect.fromRect(scrollbox.getBoundingClientRect()).map(Math.round);
>+    if (doffset.y < 0 && rect.bottom < window.innerHeight)
>+      y = Math.max(doffset.y, rect.bottom - window.innerHeight);
>+    if (doffset.y > 0 && rect.top > 0)
>+      y = Math.min(doffset.y, rect.top);
>+
>+    doffset.subtract(x, y);
>+    return new Point(x, y);
>   },

I'm not sure we really need the scrollbox var.


>           <!-- Content viewport -->
>-          <html:div id="tile-container" style="overflow: hidden;"/>
>+          <scrollbox id="tile-container-container" style="overflow: hidden;" class="window-width window-height">
>+            <!-- Content viewport -->
>+            <html:div id="tile-container" style="overflow: hidden;"/>
>+          </scrollbox>

Could we rename "tile-container-container" by "content-scrollbox"?


p.s: I've not test the performance

r+ with these changes.
Attachment #406576 - Flags: review?(21) → review+
Attached patch Code reviewSplinter Review
Got Jay to check the panning perf independently, I think we're ready to land assuming this patch looks good.
Attachment #406576 - Attachment is obsolete: true
Attachment #407148 - Flags: review?(21)
pushed:
https://hg.mozilla.org/mobile-browser/rev/56fc9874ba4b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: B3 → B5
awesome stuff, verified FIXED on builds:

Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.2b1pre) Gecko/20091021 Fennec/1.0a4pre

and

Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:1.9.2b1pre) Gecko/20091021
Fennec/1.0b5pre

and

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a1pre) Gecko/20091021
Fennec/1.0b5pre
Status: RESOLVED → VERIFIED
Added to litmus: https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=9770

in-litmus+
Flags: in-litmus? → in-litmus+
Component: General → Panning/Zooming
You need to log in before you can comment on or make changes to this bug.