510981 - Clean up bookmark popup positioning code

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Bookmarks, defect)

Description

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

Currently is positions the popup based on the position of the sidebar and screen width. Should probably be anchored to the right of the tile-container instead (see bug 494491 comment 7).

Comment 1

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

Attachment: patch

-change to using a hardcoded width/height
-position the popup on opening, and invalidate when the window is resized
-remove margin/padding on the box so that the positioning works better

Comment 2

[Stuart Parmenter]

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

getBoundingClientRect coords need to be rounded.  in show top and left should both be wholly rounded

Comment 3

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

also don't want to be using hardcoded width/height. I have an updated patch in my tree, but it has some issues I need to sort out.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Comment 7

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

Comment on attachment 414739 [details] [diff] [review]
Patch

>diff -r 914a3f23bbb2 chrome/content/browser-ui.js

>-    // bookmark popup
>-    let bookmarkPopup = document.getElementById("bookmark-popup");
>-    let bookmarkPopupW = windowW / 4;
>-    bookmarkPopup.height = windowH / 4;
>-    bookmarkPopup.width = bookmarkPopupW;
>-    let starRect = this.starButton.getBoundingClientRect();
>-    const popupMargin = 10;
>-    bookmarkPopup.top = Math.round(starRect.top) + popupMargin;
>-    bookmarkPopup.left = windowW - this.sidebarW - bookmarkPopupW - popupMargin;

>diff -r 914a3f23bbb2 chrome/content/browser.xul

>-    <vbox id="bookmark-popup" hidden="true" class="dialog-dark" align="center">
>+    <vbox id="bookmark-popup" hidden="true" class="dialog-dark" align="center" top="78" left="0">

I think I'd rather keep the setting of .top in sizeControls rather than hardcoding it here. It can be changed to use BookmarkPopup.box rather than calling getElementById directly, though.

>   show : function (aAutoClose) {

>+    let [,,,ritew] = Browser.computeSidebarVisibility();
>+    this.box.left = window.innerWidth - (this.box.getBoundingClientRect().width + ritew + margin);

Is it really necessary to do this calculation each time the popup is shown? Can't the popup only be displayed while the entire sidebar is visible, which means that a constant offset should work? I don't understand what causes the current misplacement issues, and I guess I'd like to before changing to use this technique.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #7)
> >+    let [,,,ritew] = Browser.computeSidebarVisibility();
> >+    this.box.left = window.innerWidth - (this.box.getBoundingClientRect().width + ritew + margin);
> 
> Is it really necessary to do this calculation each time the popup is shown?
> Can't the popup only be displayed while the entire sidebar is visible, which
> means that a constant offset should work? I don't understand what causes the
> current misplacement issues, and I guess I'd like to before changing to use
> this technique.

As said on IRC I'm a bit worried that extensions developers want to call it from extensions even if the right sidebars is hidden.

Comment 9

[Axel Hecht [:Pike]]

This is solving problems we have in at least one localization with the text not fitting 1/4 of the screen width in bookmark-popup.

Comment 10

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

(In reply to comment #8)
> As said on IRC I'm a bit worried that extensions developers want to call it
> from extensions even if the right sidebars is hidden.

OK, let's leave that part as is then.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

Adressed comments

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.3

really adressed comments

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Oups. I've forgot to include this change in my patch.

Comment 14

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

pushed:
https://hg.mozilla.org/mobile-browser/rev/f51cb1442796

Comment 15

[Aakash Desai [:aakashd]]

verified FIXED (used ru and es-**) on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b5pre) Gecko/20091202 Firefox/3.6b5pre Fennec/1.0b6pre

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091202 Firefox/3.7a1pre Fennec/1.0b5