474804 - Fennec UI should dynamically resize to fit screen (WinCE)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Fennec doesn't resize to fit the given screen size of a device. Portrait devices are particularly broken.

Comment 1

[Madhava Enros [:madhava]]

This is partly a problem with the graphics that we use, too -- it's not just a matter of making horizontal bars (like the titlebar) less wide, though that has to happen.  The icons, buttons, and other graphics have to be smaller to work on screens where the pixels are bigger (lower resolution).

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

This fixes a bunch of bugs with resizing of the window, for both the chrome and the widgetstack (resizing into portrait in particular). It causes a bug with the initial pan that I still need to investigate.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Summary of changes:
-changed viewportUpdateHandler arguments ("old bounds" weren't actually used, and were incorrect because we didn't apply the offsets to them, so I turned it into a "boundsSizeChanged" boolean)
-removed some of the logging code that's cluttering the source (I think it's easy enough to just temporarily throw in my own dump()s if I need to debug)
-moved the call to _updateWidgets after the _addNewWidget loop in the WidgetStack init, since it only needs to run once when all widgets are added
-made sure pannableBounds is updated properly when the rects it depends on (viewportBounds/pannableBounds) change
-added support for detecting widget size changes (by factoring out _updateWidgetRect)
-made _adjustViewingRect not attempt to "fix up" the viewing rect if that's not possible (was affecting the initial startup case when the window was larger than the default viewport size)
-fixed a bug in _updateViewportOverflow that caused to end up with bogus viewportOverflow values when the window/stack were larger than the viewport
-added some UI elements to sizeControls so that they're resized properly when the window resizes
-removed the browserUI resize handler and just had the browser.js handler call sizeControls directly (need to ensure that controls are sized before calling ws.updateSize)

_updateWidgetRect adjusting the viewport rect without taking into account the vpoffset* attributes was what was causing my panning bug with the earlier patch. I fixed that by having it just ignore the viewport for now.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 360164 [details] [diff] [review]
patch

This looks ok, but a few issues:

>-      this._viewportUpdateHandler.apply(window, [vwib]);
>+      // notify the viewportUpdateHandler of the bounds change
>+      this._viewportUpdateHandler(vwib, true);

This needs to keep using the apply syntax, otherwise the scope will be wrong in _viewportUpdateHandler.
> 
>+  _updateWidgetRect: function(state) {
>+    // don't need to support updating the viewport rect at the moment
>+    // (we'd need to duplicate the vptarget* code from _addNewWidget if we did)
>+    if (state == this._viewport)
>+      return;
>+
>+    let w = state.widget;
>+    let x = w.getAttribute("left") || 0;
>+    let y = w.getAttribute("top") || 0;
>+    let rect = w.getBoundingClientRect();
>+    state.rect = new wsRect(parseInt(x), parseInt(y),
>+                            rect.right - rect.left,
>+                            rect.bottom - rect.top);
>+    if (w.hasAttribute("widgetwidth") && w.hasAttribute("widgetheight")) {
>+      state.rect.width = parseInt(w.getAttribute("widgetwidth"));
>+      state.rect.height = parseInt(w.getAttribute("widgetheight"));
>+    }
>+
>+    if (w.hasAttribute("constraint")) {
>+      let cs = w.getAttribute("constraint").split(",");
>+      for each (let s in cs) {
>+        if (s == "ignore-x")
>+          state.ignoreX = true;
>+        else if (s == "ignore-y")
>+          state.ignoreY = true;
>+        else if (s == "sticky")
>+          state.sticky = true;
>+        else if (s == "frozen") {
>+          state.frozen = true;
>+        } else if (s == "vp-relative")
>+          state.vpRelative = true;
>+      }
>+    }
>+  },

Adding this helper function is fine, but why put the attribute checking stuff in here?  It should only need to be set once, when the state is first created.. so I'd leave that part back in the function it was removed from.

Looks fine other than that.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #5)
> This needs to keep using the apply syntax, otherwise the scope will be wrong in
> _viewportUpdateHandler.

Well, arguably "window" isn't really any better as a thisobj; our viewportUpdateHandler still needs to call the actual method via a closure. I guess it doesn't really matter, so I'll leave it as it was. I'll make that other change as well.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Attachment: better patch

I found a couple of issues with the last patch when resizing the window on the n810 (_updateViewportOverflow doesn't handle the case where toolbars are panned into view, since it takes into account the current rects to determine the overflow rect). Those parts aren't actually necessary for non-standard window sizes to work, though, since we never end up changing the overflowRect, so I've just commented them out for now.

I found another issue while debugging the existing "resizing while panned to the right" problem - panBy's arguments don't behave the way I was expecting them to (negative coordinates mean moving the viewingrect right/down). Need to investigate that further, because panBy is used by _adjustViewingRect, but that's probably better off in a seperate bug, since its an existing issue.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(Not exactly related to this bug, but I wanted to get this written down somewhere...)

I think part of the problem with resizing while panned to the right is that the cached _panRegionOffset in dragStartOffsets are later compared to panRegionOffsets calculated using a viewingRect with a different size (from after the resize). I tried doing the _adjustViewingRect pan before resizing _viewingRect, in addition to fixing the signs in the call to panBy, and that fixed some of the symptoms, but there were still problems with the viewportInnerBounds not being updated properly.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

One of things that I was looking at last was that I think there are really too many rectangles being tracked, and that we can just compute the values that we need based on only 2, maybe 3.  I'll be up there week after next, if you have some time maybe we can sit down and just put everything up on the whiteboard and figure out if we can simplify some stuff.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Sure, sounds good. In the mean time, I think attachment 360421 [details] [diff] [review] is a net win (doesn't introduce any problems that weren't already there, as far as I can tell), so I think it's ready to land.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yeah, sounds good to me.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mobile-browser/rev/a2507677cb3d

I'm going to call this FIXED, and file some followups on the other issues.

Comment 13

[Aakash Desai [:aakashd]]

verified FIXED on build:

Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.3a1pre) Gecko/20090818 Fennec/1.0a3pre