If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

after zoom, titlebar is partially off screen

VERIFIED FIXED in fennec1.0b5

Status

Fennec Graveyard
Panning/Zooming
--
critical
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: madhava, Assigned: stechz)

Tracking

Trunk
fennec1.0b5
Dependency tree / graph
Bug Flags:
in-litmus +

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 395150 [details]
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.

Updated

8 years ago
tracking-fennec: ? → 1.0+
Duplicate of this bug: 511942

Updated

8 years ago
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)

Updated

8 years ago
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
(Assignee)

Comment 8

8 years ago
Created attachment 403889 [details] [diff] [review]
Add a third scrollbox

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
(Assignee)

Comment 10

8 years ago
Created attachment 404130 [details] [diff] [review]
vingetun tweaks

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)
(Assignee)

Comment 11

8 years ago
Created attachment 404131 [details] [diff] [review]
Correct patch for vingetun tweaks

Sorry for spam, reuploaded original attachment accidentally.
Attachment #404130 - Attachment is obsolete: true
Attachment #404131 - Flags: review?(pavlov)
Attachment #404130 - Flags: review?(pavlov)
(Assignee)

Updated

8 years ago
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.
(Assignee)

Comment 14

8 years ago
> 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.
(Assignee)

Updated

8 years ago
Blocks: 520872
(Assignee)

Updated

8 years ago
Depends on: 520910
(Assignee)

Updated

8 years ago
Depends on: 520913
(Assignee)

Comment 16

8 years ago
Created attachment 404964 [details] [diff] [review]
Code review changes

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)
(Assignee)

Updated

8 years ago
Attachment #404964 - Flags: review?(pavlov) → review?(mark.finkle)
(Assignee)

Comment 17

8 years ago
Created attachment 406261 [details] [diff] [review]
Sync with dependent patches
Attachment #404964 - Attachment is obsolete: true
Attachment #406261 - Flags: review?(pavlov)
Attachment #404964 - Flags: review?(mark.finkle)
(Assignee)

Comment 18

8 years ago
Created attachment 406576 [details] [diff] [review]
Bit rot
Attachment #406261 - Attachment is obsolete: true
Attachment #406576 - Flags: review?(gavin.sharp)
Attachment #406261 - Flags: review?(pavlov)
(Assignee)

Updated

8 years ago
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+
(Assignee)

Comment 20

8 years ago
Created attachment 407148 [details] [diff] [review]
Code 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)
Attachment #407148 - Flags: review?(21) → review+
pushed:
https://hg.mozilla.org/mobile-browser/rev/56fc9874ba4b
Status: NEW → RESOLVED
Last Resolved: 8 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.