Closed Bug 1264337 Opened 8 years ago Closed 8 years ago

We lost the sync/tabs-from-other-devices tab icon in the awesomebar popup redesign

Categories

(Firefox :: Address Bar, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 48
Tracking Status
firefox48 --- verified

People

(Reporter: adw, Assigned: adw)

References

Details

Attachments

(1 file, 1 obsolete file)

Tabs from other devices should have the same icon as switch-to-tab results.
Bug 1262588 - Keep favicons in awesomebar popup aligned with urlbar's identity icon. r?mak

Review commit: https://reviewboard.mozilla.org/r/46543/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46543/
Attachment #8741515 - Flags: review?(mak77)
The redesign bug removed some CSS that matched actiontype$=tab, and for styling the type icon it also replaced actiontype with just type.

Review commit: https://reviewboard.mozilla.org/r/46545/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46545/
Comment on attachment 8741514 [details]
MozReview Request: Bug 1264337 - Add the sync/tabs-from-other-devices tab icon back to awesomebar results. r=mak

Didn't mean to post this.
Attachment #8741514 - Attachment is obsolete: true
Comment on attachment 8741515 [details]
MozReview Request: Bug 1264337 - Add the sync/tabs-from-other-devices tab icon back to awesomebar results. r?mak

https://reviewboard.mozilla.org/r/46545/#review43339

r=me with the issue fixed.

::: browser/themes/linux/browser.css:1195
(Diff revision 1)
>    list-style-image: url("chrome://browser/skin/places/tag.png");
>    width: 16px;
>    height: 16px;
>  }
>  
> -.ac-type-icon[type=switchtab] {
> +.ac-type-icon[type=switchtab],

OFF TOPIC:
these should probably be ~= rather than =
It currently works cause we delete unused types at http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/autocomplete.xml#1817

We may even decide to stop deleting types and isntead use "contains" everywhere in css. What do you think?
That would also simplify other code in _adjustAcItem, where we have to keep using 2 lists (original and filtered).

If you agree, please file a separate bug, otherwise, just let me know. Regardless it's not something we should fix in this bug.

::: toolkit/content/widgets/autocomplete.xml:1880
(Diff revision 1)
>                this.classList.add("overridable-action");
>                displayUrl = this._unescapeUrl(action.params.url);
>                let desc = this._stringBundle.GetStringFromName("switchToTab2");
>                this._setUpDescription(this._actionText, desc, true);
>              } else if (action.type == "remotetab") {
> +              type = "remotetab";

I think we need this only because UnifiedComplete.js is not setting style to "action remotetab"?
Otherwise we'd also need it for switchtab.
See http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/UnifiedComplete.js#1259
It would be more consistent if we'd set it in the same way for both, so I'd suggest to change UnifiedComplete.

I'd suggest to re run tests on Try if you change this, we have some tests checking style that may break.
Attachment #8741515 - Flags: review?(mak77) → review+
Comment on attachment 8741514 [details]
MozReview Request: Bug 1264337 - Add the sync/tabs-from-other-devices tab icon back to awesomebar results. r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46543/diff/1-2/
Attachment #8741514 - Attachment description: MozReview Request: try: -b do -p all -u all -t none → MozReview Request: Bug 1264337 - Add the sync/tabs-from-other-devices tab icon back to awesomebar results. r=mak
Attachment #8741514 - Attachment is obsolete: false
Attachment #8741514 - Flags: review?(mak77)
Attachment #8741515 - Attachment is obsolete: true
Comment on attachment 8741514 [details]
MozReview Request: Bug 1264337 - Add the sync/tabs-from-other-devices tab icon back to awesomebar results. r=mak

Why does `hg push review` with r=mak sometimes flag the review again and sometimes not...
Attachment #8741514 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #4)
> We may even decide to stop deleting types and isntead use "contains"
> everywhere in css. What do you think?
> That would also simplify other code in _adjustAcItem, where we have to keep
> using 2 lists (original and filtered).

Hmm, that seems OK, yeah.  I hesitate because conceptually each entry should have a single type, not multiple possible types.  At least that's how I've been thinking about it, but maybe that's wrong or just unnecessary.  I'll file a bug for it anyway.

> I think we need this only because UnifiedComplete.js is not setting style to
> "action remotetab"?

Exactly right, good point.  I found one test that was broken by this change.  I'll push this new commit to try before landing -- try's closed right now.
https://hg.mozilla.org/mozilla-central/rev/42a0a7f409d1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
(In reply to Drew Willcoxon :adw from comment #6)
> Comment on attachment 8741514 [details]
> MozReview Request: Bug 1264337 - Add the sync/tabs-from-other-devices tab
> icon back to awesomebar results. r=mak
> 
> Why does `hg push review` with r=mak sometimes flag the review again and
> sometimes not...

I suspect it depends on how you modify the patch, likely if you use histedit or somehow lose the review id in the commit message, you may end up creating a new request.
I usually just use hg commit --amend and never had problems
The icon for synced tabs from other devices is now properly displayed on Aurora 48.0a2, build ID 20160508004021.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.