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

VERIFIED FIXED in Firefox 48

Status

()

Firefox
Address Bar
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: adw, Assigned: adw)

Tracking

Trunk
Firefox 48
Points:
---

Firefox Tracking Flags

(firefox48 verified)

Details

MozReview Requests

()

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

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Tabs from other devices should have the same icon as switch-to-tab results.
(Assignee)

Comment 1

2 years ago
Created attachment 8741514 [details]
MozReview Request: Bug 1264337 - Add the sync/tabs-from-other-devices tab icon back to awesomebar results. r=mak

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)
(Assignee)

Comment 2

2 years ago
Created attachment 8741515 [details]
MozReview Request: Bug 1264337 - Add the sync/tabs-from-other-devices tab icon back to awesomebar results. r?mak

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/
(Assignee)

Comment 3

2 years ago
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+
(Assignee)

Comment 5

2 years ago
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)
(Assignee)

Updated

2 years ago
Attachment #8741515 - Attachment is obsolete: true
(Assignee)

Comment 6

2 years ago
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+
(Assignee)

Comment 7

2 years ago
(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.
(Assignee)

Comment 8

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=89e43c97c06c
(Assignee)

Comment 9

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/42a0a7f409d1

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/42a0a7f409d1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
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
status-firefox48: fixed → verified
You need to log in before you can comment on or make changes to this bug.