In the Australis design, the awesomebar result width extends to take up all available horizontal space. The sole exception being the requirement to align the left edge of the results with the text entered in the awesomebar, which leaves some empty white space on the left side. This requirement is also waived when the distance between the left edge of the toolbar/window and the left edge of the location bar is "big". In the Photon design, the awesomebar result width should be limited by the awesomebar width in all cases. This is a return to the levels of information density we had with the awesomebar panel that was constrained by the location bar, without actually constraining the panel, just the result width. The panel will still extend to the edges of the window. The target for this change is 57, so if we end up doing it sooner, it should be hidden behind the photon pref or build flag.

There is a very clear regression here, where in our previous case the width was limited, but information was spread across 2 lines. It's not just returning to where we were. We onelined all the info since we had far more space, now a large part of that info may be cut by shrinking to the input field width. It's true that we *may* gain the search bar space, but that's not certain (user may re-enable it) and there's the big "flexible spaces" incognita. The Safari UX for this case is horrible, imho. We should not copy it without a deep understanding of the many downsides it has. Maybe we could find a better compromise, depending on the size of the location bar input field?

It would have been a regression if "more information" === "better UX", while we have plenty of data that point to the contrary. I can give you more context later.

UX suggested adding a max-width in the result title, so we don't lose the URL entirely in narrow window sizes.

phlsa also added: We can definitely drop the constraint on the right side for smaller windows. I'm less sure about the left side. Maybe it is just a matter of two different break points? It would be awesome to have those as independent about:config prefs so that we can play around with the actual values.

if we'd margin the richlistbox as suggested in bug 1402272, likely it would also introduce a simple way to fix this.

I discussed this with Marco, we're trying the approach in bug 1402272 as it's getting us less intermittent behavior and clear performance wins.

I'm not sure the new approach of bug 1402272 will fix this, probably not, so reopening.

This should be a fairly straightforward fix, just updating the over/underflow logic where we determine how much space each item has. The one corner case that sticks out in my mind is when we give up trying to align the icons. In that case, the items are aligned with the left edge of the popup, so I guess that they should take up the full popup width. That's a nice concession to users who don't like what this bug is doing.

WIP. I'm finding that we're still not truncating results properly, which is more noticeable with this bug's intended design. Switch-to-tab results seem never to be truncated at all. And if I type a single character, backspace, and then type that same character again, results that were truncated the first time the popup opened are no longer truncated at all. This patch comments out some checks so that over/underflow is always handled at the sites where that now conditionally happens, and that fixes it, but I need to think more about it. That's basically the reason this is only a WIP for now. Also, and I mentioned this in a previous comment, but I think we should let results take up the full popup width if the urlbar is at the left edge of the window (i.e., if siteIconStart is very small), even if the urlbar is smaller than the window width, and that's what this patch does. Because (1) I think part of the design goal is to center the result text along with the centered urlbar (IMO anyway), and when the urlbar is no longer centered, it looks wrong for there to be a bunch of empty space on the right side of the popup. (2) I think it's a nice thing to do for power users.

Comment on attachment 8919767 [details] Bug 1381427 - Proof of concept without additional reflows, keeping existing bugs. We should decide on an approach that we could uplift to Firefox 57. The first is the one that Drew explored. This gives correct results all the time, but it adds many more sync reflows, potentially regressing performance even in cases where there is no overflow. We'd have to update the tests and uplift these changes. The second is the one followed by this proof of concept. It doesn't look great because it still has the current bug for which some results are truncated without the ellipsis, but on the other hand it doesn't have the above performance issue. Marco, what do you think?

We should first clarify if this still a priority for 57. It sounds riskier than expected. I don't think we can pay the reflows cost, Drew's approach was just a catch'all test to see if the problem was a missing handleUnderOverflow, but it's not necessary, we could find where we are missing a specific overflow handling and fix just that. The alternatitve approch may even be fine, but it still looks risky, maybe even riskier because it seems to potentially touches any autocomplete field, rather than being limited to the urlbar (inherited from our horrible situation where everything shares the same code for very different purposes). Personally I'd go for Drew approach and chase the few cases where we're not handling overflow. Though, personally I'm not really convinced this can safely make 57.

I think Philipp wasn't considering this as critical for 57.

(In reply to Marco Bonardo [::mak] from comment #14) > I don't think we can pay the reflows cost, Drew's approach was just a > catch'all test to see if the problem was a missing handleUnderOverflow, but > it's not necessary, we could find where we are missing a specific overflow > handling and fix just that. To clarify, hoping that I understand it correctly, what this patch does is to calculate and set max widths also in case the overflow is not detected by the related DOM event. This is necessary if the result would naturally end between the edge of the text field and the edge of the browser window. I think this is what causing the sync reflows, so it's not viable if we don't want the reflows to happen. > The alternatitve approch may even be fine, but it still looks risky, maybe > even riskier because it seems to potentially touches any autocomplete field, Both patches touch the autocomplete-richlistitem binding, that potentially applies to other places too, though only mine has to touch the login warnings that the other didn't have to change. > Though, personally I'm not really convinced this can safely make 57. It seems we're left with two equally risky approaches, yes.

(In reply to :Paolo Amadini from comment #16) > To clarify, hoping that I understand it correctly, what this patch does is > to calculate and set max widths also in case the overflow is not detected by > the related DOM event. Hm, I could have underevaluated the problem. I thought Drew's reasons to comment out the conditioning was to investigate why in certain cases we were not limiting the size, but instead looks like it was the chosen approach to trim the results, because we can't rely on overflow events, since we don't overflow. Is this correct? Then, your approach looks more palatable, sorry for the misreading. My fear about the risk is mostly that you are changing the structure of the rli, and since this code is used everywhere the risk of regressions is a bit higher. The approach looks good: * calculate the max width, we can do this using getBoundsWithoutFlushing on the input field * set the max width on something, so when the text overflows we get overflow events If we'd align information in the popup we could likely define a specific percentage of space for each info, when the full width is known calculate max-width for each text box, and set them, Then they'd naturally overflow and we'd have to do nothing. Though, our layout is more complex because we overflow based on contents (like "this page has no title, so let the url take full space). This adds challenges. Honestly we discussed in the past about aligning titles and urls, and since it would make this so much easier, could it be worth reintroducing that discussion? Otherwise, I think your approach is the way to go, and I'm sorry that I initially mis-interpreted it. We'll have to be careful about introducing regressions due to the structural change.

Note that if we should ever decide to align information, we'll have to do something about action and tags, since we can't reserve space for things that often are not present. So, while it would make fixing this trivial, it would introduce other challenges.

I've found another solution that I didn't think about earlier, using a transparent border. I think this may be upliftable to Firefox 57 after adequate testing. I followed Drew's suggestion of introducing some logic for avoiding the results to become too small or the margin too big.

This fails the reflow test, though. I'm still not sure why, because the only thing added is the end border.

what's the diff for the reflow test? is it intermittent? Is the end margin usable?

That was just because I didn't check when the values were equal. This was in an earlier version but got lost.

Full tryserver build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a9ed020a04089bc106a091bbaadacc6fc64664f

Full tryserver build from Beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=436950e90afa7aef6f47570bb4bbd58c13923df5

Philipp, here is a link for a Beta build to try on Mac OS X: https://public-artifacts.taskcluster.net/Db869yDRQfCLca8agFgkhw/0/public/build/target.dmg Let me know if you need to try on other platforms.

Comment on attachment 8920565 [details] Bug 1381427 - Result width should often be constrained by the width of the location bar. https://reviewboard.mozilla.org/r/191570/#review196980 I hope one day we can find a cleaner way to do this, in the meanwhile I'll gladly test this in Nightly. ::: browser/base/content/urlbarBindings.xml:1901 (Diff revision 2) > // according to whatever's in the CSS. > - this.siteIconStart = undefined; > + this.margins = undefined; > } > > - // Now that siteIconStart has been set, start adding items (via > + // Now that the margins have been set, start adding items (via > // _invalidate). nit: can this comment be onelined now? ::: browser/themes/shared/urlbar-autocomplete.inc.css:20 (Diff revision 2) > .autocomplete-richlistitem { > min-height: 30px; > font: message-box; > border-radius: 2px; > padding-inline-start: var(--item-padding-start); > + border-inline-end: var(--item-padding-end) solid transparent; worth a comment explaining why we use the border. ::: toolkit/content/widgets/autocomplete.xml:1390 (Diff revision 2) > </method> > > - <!-- The x-coordinate relative to the leading edge of the window of the > - items' site icons (favicons). --> > - <property name="siteIconStart" > - onget="return this._siteIconStart;"> > + <!-- This is set by urlbarBindings.xml either to undefined or to a new > + object containing { start, end } margin values in pixels. These are > + used to align the results to the input field. --> > + <property name="margins" This property is no more used by autocomplete.xml, so please move it to urlbarbindings.xml and update the comment ::: toolkit/content/widgets/autocomplete.xml:1394 (Diff revision 2) > - <property name="siteIconStart" > - onget="return this._siteIconStart;"> > + used to align the results to the input field. --> > + <property name="margins" > + onget="return this._margins;"> > <setter> > <![CDATA[ > - if (val != this._siteIconStart) { > + if (val != this._margins && nit: to make the condition a bit more readable, you could add an early bailout: if (val == this._margins) return val; if (... ::: toolkit/content/widgets/autocomplete.xml:1406 (Diff revision 2) > + 6 // .ac-site-icon margin-inline-start > + 16 // .ac-site-icon width > + 6; // .ac-site-icon margin-inline-end > - let actualVal = Math.round(val) - paddingInCSS; > - this.style.setProperty(varName, actualVal + "px"); > + let actualVal = Math.round(val.start) - paddingInCSS; > + this.style.setProperty("--item-padding-start", actualVal + "px"); > + this.style.setProperty("--item-padding-end", val.end + "px"); we should round val.end too, the rect doesn't always return ints

(In reply to Marco Bonardo [::mak] from comment #28) > nit: can this comment be onelined now? Still over 80 characters :-/

Pushed by paolo.mozmail@amadzone.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/20d45a0a9a7b Result width should often be constrained by the width of the location bar. r=mak

Hi Brindusa, can QA verify this in Nightly today, and maybe also try the Beta build from comment 27? We'd like an assessment of the quality of the implementation to ensure we can uplift safely to Firefox 57, which would be best to do soon. Things to try include customizing the URL bar in various ways and verifying that it's possible to get more space for the results by moving the location bar at the beginning of the toolbar, by adding many icons at the end, and sometimes by enabling the search bar, though the latter depends on how much space the search bar actually takes, since it's resizable. Left-to-right builds should be tested as well. Note that there is a known bug for which sometimes the ellipsis doesn't appear at the end of a location bar result, but regardless the results should be truncated at the expected position.

Comment on attachment 8920565 [details] Bug 1381427 - Result width should often be constrained by the width of the location bar. Approval Request Comment [Feature/Bug causing the regression]: The location bar results being limited by the input field, as in attachment 8886987 [details], is an integral part of the new Photon location bar design and was present since the beginning in all the UX mockups. We'd really have wanted to have this ready earlier, and that's why we worked on bug 1402272, but this last bit was delayed due to unforeseen circumstances. On the upside, I was lucky enough to find a solution that greatly reduces the risk associated with the change. This builds on the work done in bug 1402272. [User impact if declined]: Leaving the design of the most prominent UI area incomplete in Firefox 57, and implementing it in Firefox 58, is likely to create more confusion than a late uplift to Firefox 57. [Is this code covered by automated tests?]: Yes, for interaction, sync reflows, and performance. None of the existing tests regressed or had to be changed. [Has the fix been verified in Nightly?]: I already tested this in various scenarios under Mac OS X and Windows, on a Beta tryserver build and on the latest official Nightly. I haven't observed any regressions or unexpected behavior, but we're going to have some detailed QA done. [Needs manual test from QE? If yes, steps to reproduce]: Yes, see comment 0 for the overview and comment 33 for what needs to be tested. [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: Very low risk [Why is the change risky/not risky?]: To implement this, the only code that had to be changed was specifically written to control the margins in the location bar results popup. The change does not affect other areas of the interface or anything related to the location bar input field or the popup positioning. In the very unlikely scenario where this needs to be backed out, it can be done safely. Small adjustments like the exact formula used to control the width could also be made safely later, although I don't expect this will be needed. Also note that users can reclaim more space for the results by customizing the position of the location bar or by removing the flexible spacers around it. [String changes made/needed]: None

Comment on attachment 8921022 [details] Bug 1381427 - Result width should often be constrained by the width of the location bar. This was supposed to be a new patch, but MozReview messed up the flags. Fixing things now, the approval request is in comment 34.

New build based on the latest Beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a6c20c7fe6e3546cf8cdb060a02c8854dfd3b37

(In reply to :Paolo Amadini from comment #33) > Hi Brindusa, can QA verify this in Nightly today, and maybe also try the > Beta build from comment 27? > > We'd like an assessment of the quality of the implementation to ensure we > can uplift safely to Firefox 57, which would be best to do soon. > > Things to try include customizing the URL bar in various ways and verifying > that it's possible to get more space for the results by moving the location > bar at the beginning of the toolbar, by adding many icons at the end, and > sometimes by enabling the search bar, though the latter depends on how much > space the search bar actually takes, since it's resizable. > > Left-to-right builds should be tested as well. > > Note that there is a known bug for which sometimes the ellipsis doesn't > appear at the end of a location bar result, but regardless the results > should be truncated at the expected position. Hani Yacoub, please start testing this updates.

Here are the links to the latest Beta builds from comment 37, without the installers: Windows: https://queue.taskcluster.net/v1/task/RYLZjmf7T0-VJqrTzomuqg/runs/0/artifacts/public/build/target.zip Mac: https://queue.taskcluster.net/v1/task/K36oY5m7TSSlcskAosi_vw/runs/0/artifacts/public/build/target.dmg Linux: https://queue.taskcluster.net/v1/task/cBUsJyOPSIymoJVz5x6Y-Q/runs/0/artifacts/public/build/target.tar.bz2

Looks like it doesn't apply to the branch? ---- grafting 431418:20d45a0a9a7b "Bug 1381427 - Result width should often be constrained by the width of the location bar. r=mak" merging browser/base/content/browser.xul merging browser/base/content/urlbarBindings.xml merging toolkit/content/widgets/autocomplete.xml warning: conflicts while merging browser/base/content/urlbarBindings.xml! (edit, then use 'hg resolve --mark') abort: unresolved conflicts, can't continue (use 'hg resolve' and 'hg graft --continue') ----

You should use the latest changeset from MozReview: hg import https://reviewboard-hg.mozilla.org/gecko/rev/8222bee3de48ca17eeba530bd86cf245d38f19b5 This is based on Beta, and it's what the builds from comment 39 are based on. The changeset on trunk doesn't apply cleanly because osome unused code in neighboring lines was removed.

Comment on attachment 8921022 [details] Bug 1381427 - Result width should often be constrained by the width of the location bar. Photon related, it's a bit late for this kind of change to land in beta57, but I am taking it since it was planned for 57.

Well, it looks like I timed my absence in a way that makes this request obsolete

(In reply to Marco Bonardo [::mak] from comment #17) > Hm, I could have underevaluated the problem. > I thought Drew's reasons to comment out the conditioning was to investigate > why in certain cases we were not limiting the size That's right fwiw. > but instead looks like it was the chosen approach to trim the results, > because we can't rely on overflow events, since we don't overflow. Is this > correct? That's not right fwiw. :-)

Build ID: 20171026221945 User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0 Verified as fixed on Firefox Nightly 58.0a1 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.

I have also verified this bug on 57 Beta 13 across platforms: Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x64 LTS. The only potential issue I noticed is the following: if Firefox's width is resized to the minimum and some long characters are typed in the awesome bar, the navigation bar is pushed to the left side, as well as the search suggestion panel. Please observ this .gif sceencast: https://imgur.com/a/sRIqq. I'm not sure what would be right behavior here, can you please take a look at this, Paolo?

Thanks Ciprian, I think this can be filed as a new bug, it's not a big issue but in the future we may want to polish the behavior for small window sizes.

Great! Filed bug 1413233 for the issue mentioned in comment 47. Closing this bug as verified fixed.