1477591 - Missing favicon placeholder in new all tabs panel if no Site/Fav icon is provided by the website

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[Jason Mechelynck]

Attachment: FF61 vs FF63

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180722100230

Steps to reproduce:

Disable site/fav icons and open few tabs and click list all tabs button


Actual results:

the list has empty space before tab title and empty space instead of default icons


Expected results:

Defaut site icons should be shown and there should not be any space before the title of tab starts

Comment 1

[Jason Mechelynck]

This regression is caused by 
Bug 1470947
thanks to mozregression
NI? owner

Comment 2

[Markus Jaritz [:designakt] (UX)]

Not sure why this would be a big problem, as disabling favicons is only possible through about:config, bu if caused by our work, Mark might be able to fix it.

Comment 3

[Jason Mechelynck]

(In reply to Markus Jaritz [:designakt] (UX) from comment #2)
> Not sure why this would be a big problem, as disabling favicons is only
> possible through about:config, bu if caused by our work, Mark might be able
> to fix it.

Yes this is caused after Bug 1470947 and/or Bug 1446101

By default a globe icon was shown by Firefox if site icon was missing, not provided by the site(happens alot) or disabled by the user.

Now IT's a large blank space so this is a regression.

Plus now there is a blank space before the site icon which is ugly too see as well as reduces the tab title visible which is
just bad UX.
There should be no blank space before Icons just like in FF60
see attachment.

Comment 4

[Jason Mechelynck]

Attachment: Blank Space before Icons.jpg

Comment 5

[Jason Mechelynck]

Attachment: By default a globe icon was shown if site icon was missing or disabled.jpg

Comment 6

[Markus Jaritz [:designakt] (UX)]

(In reply to Jason Mechelynck from comment #3)
> By default a globe icon was shown by Firefox if site icon was missing, not
> provided by the site(happens alot) or disabled by the user.
> 
> Now it's a large blank space so this is a regression.

Thanks for helping me understand this better. The case where there is no icon provided by the website seams to be the bigger one here, as it will be experienced by more people. I agree that this has a bad impact on the readability of the list, and should be filled with a placeholder. I updated the bug summary accordingly.

Mark, can you fix this?

Comment 7

[Pascal Chevrel:pascalc]

[Tracking Requested - why for this release]:

Comment 8

[Markus Jaritz [:designakt] (UX)]

(In reply to Jason Mechelynck from comment #3)
> Plus now there is a blank space before the site icon which is ugly too see
This space helps separate foreground from background content. 

> as well as reduces the tab title visible which is just bad UX.
The width is not about showing all text, but ensuring a balance between line length, and the ease of identifying individual tabs.
The new width is set to show an average of 66 characters, which is the optimal line length for readability, and will also ensure that by far most website-titles will be easily differentiated. (If not all 66 characters are identical.)
There is no bad UX in that.

Comment 9

[Jason Mechelynck]

Attachment: Clipboard01.jpg

(In reply to Markus Jaritz [:designakt] (UX) from comment #6)
> (In reply to Jason Mechelynck from comment #3)
> > By default a globe icon was shown by Firefox if site icon was missing, not
> > provided by the site(happens alot) or disabled by the user.
> > 
> > Now it's a large blank space so this is a regression.
> 
> Thanks for helping me understand this better. The case where there is no
> icon provided by the website seams to be the bigger one here, as it will be
> experienced by more people. I agree that this has a bad impact on the
> readability of the list, and should be filled with a placeholder. I updated
> the bug summary accordingly.
> 
> Mark, can you fix this?

Yes that is the problem and thank you.

(In reply to Markus Jaritz [:designakt] (UX) from comment #8)
> (In reply to Jason Mechelynck from comment #3)
> > Plus now there is a blank space before the site icon which is ugly too see
> This space helps separate foreground from background content. 
> 
> > as well as reduces the tab title visible which is just bad UX.
> The width is not about showing all text, but ensuring a balance between line
> length, and the ease of identifying individual tabs.
> The new width is set to show an average of 66 characters, which is the
> optimal line length for readability, and will also ensure that by far most
> website-titles will be easily differentiated. (If not all 66 characters are
> identical.)
> There is no bad UX in that.

The tab highlight is missing(it was full tab highlight) its now just a blue small line hard to locate at a glance.
The old all tab list showed more info than the current one plain and simple especially with disabling site icons which allowed more characters.

Comment 10

[Markus Jaritz [:designakt] (UX)]

(In reply to Jason Mechelynck from comment #9)
> The tab highlight is missing(it was full tab highlight) its now just a blue
> small line hard to locate at a glance.

I filed a bug for that, and it is about to be fixed:
https://bugzilla.mozilla.org/show_bug.cgi?id=1478708

Comment 11

[Mark Galoway]

(In reply to Markus Jaritz [:designakt] (UX) from comment #10)
> (In reply to Jason Mechelynck from comment #9)
> > The tab highlight is missing(it was full tab highlight) its now just a blue
> > small line hard to locate at a glance.
> 
> I filed a bug for that, and it is about to be fixed:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1478708

Still not full tab highlight just Bold text is used

Comment 12

[Markus Jaritz [:designakt] (UX)]

(In reply to Mark Galoway from comment #11)
> (In reply to Markus Jaritz [:designakt] (UX) from comment #10)
> > (In reply to Jason Mechelynck from comment #9)
> > > The tab highlight is missing(it was full tab highlight) its now just a blue
> > > small line hard to locate at a glance.
> > 
> > I filed a bug for that, and it is about to be fixed:
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1478708
> 
> Still not full tab highlight just Bold text is used

Then I probably do not understand what it is you mean with tab highlight. 
Are you talking about:
a) the highlight of the active tab 
   (which was bold text, and now is a blue line and bold text - soon)
b) the highlight of each row when hovered with the mouse or focused with the keyboard
   (which was blue, and now is gray background on the row)
c) something else
   (please specify)

Comment 13

[Mark Striemer [:mstriemer]]

Attachment: Bug 1477591 - Use default favicon in all tabs menu

Review commit: https://reviewboard.mozilla.org/r/260316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260316/

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 8996058 [details]
Bug 1477591 - Use default favicon in all tabs menu

https://reviewboard.mozilla.org/r/260316/#review268234

::: browser/themes/shared/tabs.inc.css:803
(Diff revision 1)
>  
>  .all-tabs-item[selected] {
>    box-shadow: inset 4px 0 var(--blue-40);
>  }
>  
> +.all-tabs-button > .toolbarbutton-icon {

Just use .all-tabs-button without .toolbarbutton-icon? list-style-image gets inherited.

Comment 15

[Mark Striemer [:mstriemer]]

Comment on attachment 8996058 [details]
Bug 1477591 - Use default favicon in all tabs menu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260316/diff/1-2/

Comment 16

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 00573673f9aef1f353a1b07e42ec5ffc72f4820d -d 218ec331cdec: rebasing 476657:00573673f9ae "Bug 1477591 - Use default favicon in all tabs menu r=dao" (tip)
merging browser/themes/shared/tabs.inc.css
warning: conflicts while merging browser/themes/shared/tabs.inc.css! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 17

[Mark Striemer [:mstriemer]]

Comment on attachment 8996058 [details]
Bug 1477591 - Use default favicon in all tabs menu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260316/diff/2-3/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc62d0350cbb
Use default favicon in all tabs menu r=dao

Comment 19

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/dc62d0350cbb

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 21

[CosminB]

Attachment: Bug1477591.gif

I was able to reproduce this issue on Firefox 62.0b15 (20180806191531) under Win 7 64-bit and Mac OS X 10.13.3.

This issue is verified as fixed on Firefox 63.0a1(20180807220134) under Win 7 64-bit and Mac OS X 10.13.3.

Please see the attached video.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Dão, can you request uplift if you think this fix should go into 62? If it goes too much later it'll be too late in the beta cycle.

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 8996058 [details]
Bug 1477591 - Use default favicon in all tabs menu

Approval Request Comment
[Feature/Bug causing the regression]: bug 1446101
[User impact if declined]: default favicon missing in all tabs panel
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: no
[Why is the change risky/not risky?]: trivial CSS fix
[String changes made/needed]: /

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8996058 [details]
Bug 1477591 - Use default favicon in all tabs menu

Minor CSS fix for all tabs panel, verified in nightly; let's uplift for beta 18.

Comment 25

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1382c4b50f80

Comment 26

[u554753]

Verified on 62.0b18 Build ID: 20180816151750
with Windows 10 x64, Mac OS X 10.11 and Ubuntu 18.10 x64.