Closed Bug 1446982 Opened 2 years ago Closed 2 years ago

Replace the sync device name with the URL when selected

Categories

(Firefox :: Address Bar, defect, P1)

55 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 61
Tracking Status
firefox61 --- verified
firefox62 --- verified

People

(Reporter: rfeeley, Assigned: mak)

Details

(Whiteboard: [fxsearch])

Attachments

(4 files)

Attached image device-to-url.png
Let's replace the sync device name with the URL when selected to provide more information in this specific context.
Priority: -- → P1
Whiteboard: [fxsearch]
tentatively taking, shouldn't be hard, but you never know.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Should it also happen on mouse hover?
Flags: needinfo?(rfeeley)
The patch implements :hover too, that is pending the question in comment 1.
note that whole on selection the url is shown in the input field, on hover it's not shown anywhere atm.
(In reply to Marco Bonardo [::mak] from comment #2)
> Should it also happen on mouse hover?

Whatever’s easiest. I see that we do "Search with {Search Engine}" on hover too.

It might be a little jarring, but I wouldn't make a special exception not to do it if extra work was required.
Flags: needinfo?(rfeeley)
no, it's trivial either way.
Checking with my awesomer awesomebar designer.
Marco: would we be able to see how it works with both keyboard and mouse on and get a feel for it?
Flags: needinfo?(mak77)
Attached image remotetab.png
Sure, this is a screenshot with the current patch.
From top to bottom:
1. normal entry
2. keyboard selected entry
3. mouseover entry
Flags: needinfo?(mak77)
Attachment #8966270 - Flags: review?(rfeeley)
Attached image animated.gif
I animated it to get a better feel for it. What do you think shorlander?
Flags: needinfo?(shorlander)
(In reply to Ryan Feeley [:rfeeley] from comment #0)
> Let's replace the sync device name with the URL when selected to provide
> more information in this specific context.

It seems a shame to diminish the fact that Sync provided this URL. Is there scope to do something like "www.mozilla.com, on Marco's computer" (or similar/with ellipses/truncation/whatever)?
(In reply to Mark Hammond [:markh] from comment #11)
> It seems a shame to diminish the fact that Sync provided this URL. Is there
> scope to do something like "www.mozilla.com, on Marco's computer" (or
> similar/with ellipses/truncation/whatever)?

Everything is feasible, but it would have an higher complexity, because we currently show either the url or the action, not both, thus we only have one separator in place, we'd need to add another one and fix the overflow code. At that point I'm not sure the cost would be lower than the benefit.
Imo, what we already have today is still the best compromise, on selection we already put the url in the input field, so both the url and the device are visible at the same time.
Mouseover doesn't show the url anywhere though.
maybe we need a "remote tab open" icon, instead of reusing the switch-to-tab icon, to make clear this is provided by Sync, but I don't think we have an icon that represents "this comes from another device connected to your account".
Comment on attachment 8965752 [details]
Bug 1446982 - Show the url when a remote tab entry is selected/hovered in the Address Bar.

clearing the review until we have final specs.
Attachment #8965752 - Flags: review?(adw)
Marco: I got the green light from Shorlander and Bryan. Highest compliment from both "it's fine". :)
Flags: needinfo?(shorlander)
Attachment #8966270 - Flags: review?(rfeeley)
Attachment #8965752 - Flags: review?(adw)
Comment on attachment 8965752 [details]
Bug 1446982 - Show the url when a remote tab entry is selected/hovered in the Address Bar.

https://reviewboard.mozilla.org/r/234590/#review242078

::: toolkit/content/widgets/autocomplete.xml:1848
(Diff revision 1)
>              action = action || this._parseActionUrl(originalUrl);
>              this.setAttribute("actiontype", action.type);
>  
>              if (action.type == "switchtab") {
>                this.classList.add("overridable-action");
> -              displayUrl = this._unescapeUrl(action.params.url);
> +              displayUrl = typeof(popup.input.trimValue) == "function" ?

Is it possible to factor out the trimValue() and _unescapeUrl() calls so that we only do them in one place, while you're here?  It looks like it may be, but the action.type == "visiturl" and action.type == "extension" cases may be different enough to not be worth it.
Attachment #8965752 - Flags: review?(adw) → review+
Comment on attachment 8965752 [details]
Bug 1446982 - Show the url when a remote tab entry is selected/hovered in the Address Bar.

https://reviewboard.mozilla.org/r/234590/#review242078

> Is it possible to factor out the trimValue() and _unescapeUrl() calls so that we only do them in one place, while you're here?  It looks like it may be, but the action.type == "visiturl" and action.type == "extension" cases may be different enough to not be worth it.

I converted the if/else to a switch and moved the trimming after it.
I also moved a couple assignments that were uglifying the code in current order.
hm, we're clearly not handling ALL the action types, so I'm removing the exception...
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/7cc86dc1bbc8
Show the url when a remote tab entry is selected/hovered in the Address Bar. r=adw
https://hg.mozilla.org/mozilla-central/rev/7cc86dc1bbc8
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
I've reproduced the issue using Firefox 60.0 and 61.0a1 (2018-04-01) on Win 10x64.

Verified on Firefox 61.0b3 and 62.0a1(2018-05-10) on Windows 10x64.
I can confirm that the issue is fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.