Closed Bug 1262588 Opened 9 years ago Closed 9 years ago

New awesomebar design: Icons and text in popup entries are not aligned with info icon and text in urlbar when the forward button is shown

Categories

(Firefox :: Address Bar, defect)

Product:
Firefox
Component:
Address Bar
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 48
Tracking Flags:
Tracking Status
firefox48 --- verified

People

(Reporter: adw, Assigned: adw)

References

Details

Attachments

(1 file)

MozReview Request: Bug 1262588 - Keep favicons in awesomebar popup aligned with urlbar's identity icon. r=mak
(deleted), text/x-review-board-request
mak
: review+
Details
When the forward button is shown, the favicon column in the popup entries does not align with the info icon in the urlbar, and entries' title text does not align with the urlbar text, as is the case when the forward button is not shown. I asked Stephen about it and he suggests shifting the contents of entries so that everything remains aligned.
I don't think we should star playing this game. For various reasons: 1. in the current world you always know where to look at for the beginning of results. IF we start shifting the starting point, you'll have to search for it every other time. 2. it's a less frequent case, may not be worth the code cost. I don't think it really has any tangible gain. 3. there are far more cases where this can happen, just drag any toolbar item before the locationbar. We clearly don't want to play this game in particular.
note, I knew about this issue and I explicitly didn't comment about it in the review cause I think the costs over-weight the benefits.
bug 1262828, for example.
Is the suggestion in comment 0 to do something similar to Chrome, where the panel contents have a huge padding on the left? it doesn't look very great... But I don't see how else we could handle bug 1262828 at this point.
(In reply to Marco Bonardo [::mak] from comment #4) > Is the suggestion in comment 0 to do something similar to Chrome, where the > panel contents have a huge padding on the left? it doesn't look very > great... But I don't see how else we could handle bug 1262828 at this point. I think it's reasonable to cover the cases we know to be common. In this case the forward button being shown. I don't think we should chase every possible configuration that might shift the starting point of the location bar. Because you could end up with this case: http://cl.ly/2f2F1a0H1a1L and even the old panel pop-up degrades in that case.
or we could have a limit over which we don't want to align, if we want to be more flexible.
(In reply to Marco Bonardo [::mak] from comment #6) > or we could have a limit over which we don't want to align, if we want to be > more flexible. That works for me also if it's feasible.
well I suppose we need to get the left position of the input field, at that point we could define a fixed threshold (something like left button + 2 toolbar buttons space)
s/left/forward/
Marco's suggestion in comment 6 sounds reasonable. Other solutions I can think of are: - Not align the suggestions with the urlbar contents at all and keep them at the left (actually inline start) side. => This allows suggestions to use the maximum available horizontal space. - Offset the suggestions popup, so that the suggestions align with the urlbar contents. => This minimizes visual discrepancies between both and allows to display more of the underlying page. Sebastian
Assignee: nobody → adw
Status: NEW → ASSIGNED
This keeps the site icon aligned with the identity icon when the identity icon's leading edge is < 100 points away from the window's leading edge. Arbitrary, but more than enough to cover the case where the forward button is shown. This works on Windows 10 and OS X. Need to test on Linux still.
https://reviewboard.mozilla.org/r/46013/#review42515 ::: toolkit/content/widgets/autocomplete.xml:2040 (Diff revision 1) > + <parameter name="newStart"/> > + <parameter name="popupDir"/> > + <parameter name="forceHandleOverflow"/> > + <body> > + <![CDATA[ > + newStart = newStart || 0; Hmm, probably want to use the current style.MozMarginStart instead of 0 so that the item doesn't suddenly get shifted in other toolkit apps (which don't set popup.siteIconStart). Alternatively, the popup caller could just not call this method if its _siteIconStart isn't defined.
Comment on attachment 8740760 [details] MozReview Request: Bug 1262588 - Keep favicons in awesomebar popup aligned with urlbar's identity icon. r=mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46013/diff/1-2/
https://reviewboard.mozilla.org/r/46013/#review42597 ::: toolkit/content/widgets/autocomplete.xml:2047 (Diff revisions 1 - 2) > + // Allow callers to pass in newStart=undefined but > + // forceHandleOverflow=true to force handling over/underflow. > + if (typeof(newStart) == "number") { > - let rect = this._siteIcon.getBoundingClientRect(); > + let rect = this._siteIcon.getBoundingClientRect(); > - let oldStart = popupDir == "rtl" ? rect.right : rect.left; > + let oldStart = popupDir == "rtl" ? rect.right : rect.left; > - let delta = newStart - oldStart; > + let delta = newStart - oldStart; Argh, there shouldn't be a `let` here.
Comment on attachment 8740760 [details] MozReview Request: Bug 1262588 - Keep favicons in awesomebar popup aligned with urlbar's identity icon. r=mak https://reviewboard.mozilla.org/r/46013/#review42921 ::: browser/base/content/urlbarBindings.xml:1387 (Diff revision 2) > + let siteIconStart = popupDirection == "rtl" ? identityRect.right > + : identityRect.left; > + // 100 is basically arbitrary, but it's more than enough to cover the > + // case when the forward button is shown and there's nothing before > + // the back-forward buttons. > + if (siteIconStart < 100) { I'd like to support not just the forward button, but also a couple toolbar button before the urlbar. The reason is mostly that most of the other browsers have a refresh button before the urlbar, and some of our users could prefer that setup (through an add-on). And I'd like to still give space for an add-on button (the famous frequently used add-on everyone has). In this case it's easy for us to be a little bit more flexible, provided we don't exagerate, I think 2 buttons is a good compromise. So please, try to align up to 2 toolbar button and the forward button. ::: toolkit/content/widgets/autocomplete.xml:1228 (Diff revision 2) > var controller = this.mInput.controller; > var matchCount = this._matchCount; > var existingItemsCount = this.richlistbox.childNodes.length; > > + let popupDir = > + this.ownerDocument.defaultView.getComputedStyle(this).direction; I was thinking we may set a dir property on the popup binding when openAutocompletePopup is invoked, and then just reuse that property, avoiding the continuous getComputedStyle call. In the end we only care about its value when the popup is opening, we don't care about live updating it later. All the code using it seems to happen after openAutocompletePopup (should clearly be done here and in toolkit since we override openAutocompletePopup) ::: toolkit/content/widgets/autocomplete.xml:2038 (Diff revision 2) > + the new start is different from the old start. Pass true for > + this parameter to handle it regardless. --> > + <method name="setSiteIconStart"> > + <parameter name="newStart"/> > + <parameter name="popupDir"/> > + <parameter name="forceHandleOverflow"/> I don't like that handleOverUnderflow is hidden inside this sort-of-unrelated method. I'd prefer if this would return a "changed" bool, and the consumer checks the return value and handles overflow. At that point the cases where forceHandleOverflow are true would just ignore the return value and always invoke it (with a comment, there is only one case) could rename to maybeSetSiteIconStart, to clarify one may want to check the return value.
Attachment #8740760 - Flags: review?(mak77)
Comment on attachment 8740760 [details] MozReview Request: Bug 1262588 - Keep favicons in awesomebar popup aligned with urlbar's identity icon. r=mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46013/diff/2-3/
Attachment #8740760 - Flags: review?(mak77)
Instead of using a hardcoded numeric value, this looks at the navbar DOM and aligns the icons if there are no more than two toolbarbuttons before the urlbar. Let me know what you think. It also lets the alignment be reset by setting it to undefined. Then the values in the CSS take over. That happens when there are more than two toolbarbuttons before the urlbar. Instead of adding a dir property, I think we can use this.style.direction, which was already being set. That's less obvious than a separate property though, so I'd understand if you prefer a separate property. That value is then passed on to each item by item.setAttribute("dir", dir). That way the item can avoid querying the popup or calling getComputedStyle() on itself. Finally, I noticed a subtle problem caused by this patch where sometimes items would start out too far to the right and then quickly jump into the correct position. It happened in the case where items are completely reused. The problem seemed to be that item.setSiteIconStart() was being called before the item was un-collapsed. With that changed, I can't reproduce the problem anymore. I removed the "// this method is defined on the base binding" because the redesign made it confusing, since _openAutocompletePopup is now also defined on the derived binding. Jared pointed that out to me.
Comment on attachment 8740760 [details] MozReview Request: Bug 1262588 - Keep favicons in awesomebar popup aligned with urlbar's identity icon. r=mak https://reviewboard.mozilla.org/r/46013/#review43313 ::: browser/base/content/urlbarBindings.xml (Diff revision 3) > <![CDATA[ > // initially the panel is hidden > // to avoid impacting startup / new window performance > aInput.popup.hidden = false; > > - // this method is defined on the base binding Note the comment was in browser-autocomplete-result-popup binding, that is used by #PopupAutoComplete it was not in urlbar-rich-result-popup binding, that is used by #PopupAutoCompleteRichResult For PopupAutoComplete we still use the base binding _openAutocompletePopup, so the comment was correct. I just wontfixed a bug some days ago that was trying to remove this comment :) I think to avoid confusion we could just merge openAutocompletePopup and _openAutocompletePopup in urlbar-rich-result-popup ::: browser/base/content/urlbarBindings.xml:1382 (Diff revision 3) > + // 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 alignSiteIcons = false; > + let navbar = document.getElementById("nav-bar-customization-target"); > + for (let i = 0; i < 3 && i < navbar.childNodes.length; i++) { This should be a little bit more compact: let nodes = [...document.getElementById("nav-bar-customization-target").childNodes]; let urlbarPosition = nodes.findIndex(n => n.id == "urlbar-container"); let alignSiteIcons = urlbarPosition <= 2 && nodes.slice(0, urlbarPosition) .every(n => n.localName == "toolbarbutton"); Note, I'd be fine even with a simple guessed threshold based on the platform where buttons were larger... But let's see how this goes. ::: toolkit/content/widgets/autocomplete.xml:2038 (Diff revision 3) > + relative the the leading edge of the window. > + @param newStart The new x-coordinate, relative to the leading edge of > + the window. Pass undefined to reset the icon's position to > + whatever is specified in CSS. > + @return True if the icon's position changed, false if not. --> > + <method name="setSiteIconStart"> I find setSiteIconStart confusing along with siteIconStart (that has a setter). What about calling this adjustSiteIconStart?
Attachment #8740760 - Flags: review?(mak77) → review+
Comment on attachment 8740760 [details] MozReview Request: Bug 1262588 - Keep favicons in awesomebar popup aligned with urlbar's identity icon. r=mak Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46013/diff/3-4/
Attachment #8740760 - Attachment description: MozReview Request: Bug 1262588 - Keep favicons in awesomebar popup aligned with urlbar's identity icon. r?mak → MozReview Request: Bug 1262588 - Keep favicons in awesomebar popup aligned with urlbar's identity icon. r=mak
https://hg.mozilla.org/mozilla-central/rev/009d7df1ba6d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
I have reproduced this bug with Firefox Nightly 48.0a1 (Build ID:20160406030221) on Windows 8.1, 64-bit. Verified as fixed with Firefox Release 48.0 (Build ID: 20160726073904) Mozilla/5.0 (Windows NT 6.3; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
QA Whiteboard: [bugday-20160803]
I have reproduced this with Nightly 48.0a1 (2016-04-07) on Elementary OS 64bit. This fix is now verified on Firefox release 48.0 Build ID 20160726073904 User Agent Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0 [bugday-20160803]
As this bug verified on both windows (Comment 26) and Linux (Comment 27) marking this bug as verified.
Status: RESOLVED → VERIFIED
QA Whiteboard: [bugday-20160803] → [bugday-20160803][bugday-20160810]
status-firefox48: fixed → verified
Depends on: 1353563
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: