Add flexible spaces around the URL and search bar by default and replace the bookmarks button with the library one

VERIFIED FIXED in Firefox 57

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
a month ago
5 days ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

55 Branch
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 wontfix, firefox57 verified)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

a month ago
The photon designs show these spaces around the url/search bars.

I'm less clear on whether we need to update existing profiles to have them, too, or not. Aaron?
Flags: qe-verify+
(Assignee)

Comment 1

a month ago
(In reply to :Gijs from comment #0)
> The photon designs show these spaces around the url/search bars.
> 
> I'm less clear on whether we need to update existing profiles to have them,
> too, or not. Aaron?

With needinfo this time...
Flags: needinfo?(abenson)
Remember that by design the location bar popup contents will be limited to the urlbar input field, so if we add spaces also when the search bar is visible, we'll limit the contents of the location bar popup quite a bit.

Updated

a month ago
QA Contact: gwimberly
Blocks: 1387512

Comment 3

15 days ago
Can Confirm that flexible spacers to the left of the URL bar, and To the right side of the URL bar and/or the Search Bar (if present) is a requirement.
Flags: needinfo?(abenson)
(Assignee)

Updated

15 days ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 57.1 - Aug 15
Priority: P2 → P1
(Assignee)

Updated

15 days ago
Blocks: 1365421
Comment hidden (mozreview-request)
(Assignee)

Comment 5

15 days ago
I have to admit I'm not wholly convinced this patch as-is will take care of all the test failures, given that we're increasing the number of items in the navbar and that tends to push things into overflow when not expected, plus there might be more tests that rely on the bookmarks menu button being in the navbar by default, so I pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=272e40c27527 . browser/components/customizableui/ and browser/components/places/tests/browser/ both pass for me with this patch.

Comment 6

15 days ago
mozreview-review
Comment on attachment 8894631 [details]
Bug 1383009 - add flexible spacers and library button to the navbar by default,

https://reviewboard.mozilla.org/r/165788/#review170886

::: browser/components/customizableui/CustomizableUI.jsm:407
(Diff revision 1)
> +        // The url bar is now 1 index later, add another one:
> +        let secondSpringIndex = urlbarIndex + 2;

The secondSpringIndex should only be incremented by two if we actually splice in a new spring. This currently always increments by 2.

::: browser/components/customizableui/CustomizableUI.jsm:419
(Diff revision 1)
> +      if (placements.includes("bookmarks-menu-button")) {
> +        let bmbIndex = placements.indexOf("bookmarks-menu-button");

Instead of doing this twice, we should just get the index and compare it with -1.
Attachment #8894631 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

15 days ago
Note that these test fixes fix everything except browser/base/content/test/performance/browser_urlbar_search_reflows.js, for which I need UX confirmation about what to do:

- align items in the autocomplete popup with the url bar, even with the spacer and other icons to the left of the URL bar, so creating a large amount of whitespace at the start of the autocomplete popup ( http://imgur.com/a/JfSOH )
- start-align the items so there isn't such a huge amount of whitespace, but then the items don't line up with the url bar anymore.

In the latter case, we'll have to come up with a way of making the reflow test pass.
Flags: needinfo?(shorlander)
Flags: needinfo?(bbell)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

14 days ago
shorlander said on slack we could just use 30% of the window width as the cut-off point for where we start ignoring the URL bar position and always position results at the start (left in ltr, right in rtl) edge of the window. I also added a check for a hardcoded 250px (ie, if the distance to the edge is less than that, we align with the url bar) so that we continue to align things in narrow windows. Stephen, can you confirm that works for you?
Flags: needinfo?(bbell)

Comment 14

14 days ago
mozreview-review
Comment on attachment 8895056 [details]
Bug 1383009 - fix logic for url bar autocomplete item positioning to allow it to position items in more cases,

https://reviewboard.mozilla.org/r/166184/#review171504

::: browser/base/content/urlbarBindings.xml:1878
(Diff revision 1)
> -          // at most two toolbar buttons between the window edge and the urlbar,
> -          // then consider that as "not too far."  The forward button's
> -          // visibility may have changed since the last time the popup was
> -          // opened, so this needs to happen now.  Do it *before* the popup
> -          // opens because otherwise the items will visibly shift.
> -          let nodes = [...document.getElementById("nav-bar-customization-target").childNodes];
> +          // "too far" as "more than 30% of the window's width AND more than
> +          // 250px"
> +          let boundToCheck = popupDirection == "rtl" ? "right" : "left";
> +          let inputRect = this.DOMWindowUtils.getBoundsWithoutFlushing(aInput);
> +          let startOffset = Math.abs(inputRect[boundToCheck] - documentRect[boundToCheck]);
> +          let alignSiteIcons = startOffset / width <= 0.3 || startOffset < 250;

Precisely the second term should use <=, not <, right?  To match what the comment says.
Attachment #8895056 - Flags: review?(adw) → review+
(Assignee)

Comment 15

14 days ago
Got confirmation from Stephen this works for him, so clearing ni.
Flags: needinfo?(shorlander)
Comment hidden (mozreview-request)

Comment 17

13 days ago
mozreview-review
Comment on attachment 8894879 [details]
Bug 1383009 - fix tests, BrowserUITelemetry and pocket to deal with the changes to the navbar,

https://reviewboard.mozilla.org/r/166036/#review171966

::: browser/components/uitour/test/browser_UITour_availableTargets.js
(Diff revision 2)
>    return [
>      "accountStatus",
>      "addons",
>      "appMenu",
>      "backForward",
> -    "bookmarks",

Why does adding the flexible spacers cause us to remove the "bookmarks" item. That seems related to some other work.

::: browser/extensions/pocket/bootstrap.js:410
(Diff revision 2)
>      for (let window of browserWindows()) {
>        for (let id of ["panelMenu_pocket", "menu_pocket", "BMB_pocket",
>                        "panelMenu_pocketSeparator", "menu_pocketSeparator",
>                        "BMB_pocketSeparator", "appMenu-library-pocket-button"]) {
> -        let element = window.document.getElementById(id);
> +        let element = window.document.getElementById(id) ||
> +                      window.gNavToolbox.palette.querySelector("#" + id);

Similar question, why does adding flexible spacers cause these to end up in the palette?

I understand if they were in the overflow menu, but that doesn't put them in the palette.
(Assignee)

Comment 18

13 days ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #17)
> Comment on attachment 8894879 [details]
> Bug 1383009 - fix tests, BrowserUITelemetry and pocket to deal with the
> changes to the navbar,
> 
> https://reviewboard.mozilla.org/r/166036/#review171966
> 
> ::: browser/components/uitour/test/browser_UITour_availableTargets.js
> (Diff revision 2)
> >    return [
> >      "accountStatus",
> >      "addons",
> >      "appMenu",
> >      "backForward",
> > -    "bookmarks",
> 
> Why does adding the flexible spacers cause us to remove the "bookmarks"
> item. That seems related to some other work.

I ended up also replacing the bookmarks menu button with the library button in this bug (originally bug 1365421). You reviewed that in the first patch, but it was a while back. :-)

I forgot to update the summary here, and I made the other bug dep on this one, but it's probably better to just dupe it over, so I'll do that in a second.

Note that the reason I did this in 1 patch is because it's basically manipulating the exact same set of defaults, and requires similar migration code, so I anticipated that some tests and potentially other places would otherwise just need updating twice, potentially making things more difficult by having to cope with an intermediate state. So it seemed simplest to just do it in one bug. Whether that was the right decision in hindsight... maybe not. But here we are. :-)

> ::: browser/extensions/pocket/bootstrap.js:410
> (Diff revision 2)
> >      for (let window of browserWindows()) {
> >        for (let id of ["panelMenu_pocket", "menu_pocket", "BMB_pocket",
> >                        "panelMenu_pocketSeparator", "menu_pocketSeparator",
> >                        "BMB_pocketSeparator", "appMenu-library-pocket-button"]) {
> > -        let element = window.document.getElementById(id);
> > +        let element = window.document.getElementById(id) ||
> > +                      window.gNavToolbox.palette.querySelector("#" + id);
> 
> Similar question, why does adding flexible spacers cause these to end up in
> the palette?
> 
> I understand if they were in the overflow menu, but that doesn't put them in
> the palette.

The bookmarks menu button is now in the palette by default. Rather than trying to track when it's inserted/removed and updating it then, I thought it would be simplest to just cope with it being in the palette and adding/removing items to it there.
Summary: Add flexible spaces around the URL and search bar by default → Add flexible spaces around the URL and search bar by default and replace the bookmarks button with the library one
(Assignee)

Updated

13 days ago
No longer blocks: 1365421
Duplicate of this bug: 1365421

Comment 20

13 days ago
mozreview-review
Comment on attachment 8894879 [details]
Bug 1383009 - fix tests, BrowserUITelemetry and pocket to deal with the changes to the navbar,

https://reviewboard.mozilla.org/r/166036/#review172320
Attachment #8894879 - Flags: review?(jaws) → review+

Comment 21

13 days ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 0a038a63bd9a -d 387b09897512: rebasing 412704:0a038a63bd9a "Bug 1383009 - add flexible spacers and library button to the navbar by default, r=jaws"
merging browser/base/content/browser.xul
merging browser/components/customizableui/CustomizableUI.jsm
merging browser/components/customizableui/test/browser_984455_bookmarks_items_reparenting.js
merging browser/components/places/tests/browser/browser_toolbarbutton_menu_context.js
warning: conflicts while merging browser/base/content/browser.xul! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 25

13 days ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b9516e57add6
add flexible spacers and library button to the navbar by default, r=jaws
https://hg.mozilla.org/integration/autoland/rev/23ddae2ce26d
fix tests, BrowserUITelemetry and pocket to deal with the changes to the navbar, r=jaws
https://hg.mozilla.org/integration/autoland/rev/4f5ee0246c86
fix logic for url bar autocomplete item positioning to allow it to position items in more cases, r=adw
https://hg.mozilla.org/mozilla-central/rev/b9516e57add6
https://hg.mozilla.org/mozilla-central/rev/23ddae2ce26d
https://hg.mozilla.org/mozilla-central/rev/4f5ee0246c86
Status: ASSIGNED → RESOLVED
Last Resolved: 12 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1389484
(Assignee)

Updated

12 days ago
Depends on: 1389505
(Assignee)

Updated

11 days ago
Depends on: 1389573
Verified on Windows, Ubuntu, and Mac.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+

Updated

11 days ago
Depends on: 1389658
Screenshots:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=253a8560dc34456d2e8a13065e4b3eb5ecf6704f&newProject=mozilla-central&newRev=f74094603063c92b710c0bae0bb5c092b915e92c
You need to log in before you can comment on or make changes to this bug.