Bookmark star button is not displayed on about: pages

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: giul.mus, Assigned: adw)

Tracking

57 Branch
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 verified)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(1 attachment)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170904144521

Steps to reproduce:

Type `about:preferences` in the address bar.


Actual results:

No favourite button is shown.


Expected results:

The favourite button should have been shown.
Severity: normal → minor
Component: Untriaged → Preferences
[Tracking Requested - why for this release]: Regression

Oddly, mouse menu works fine, as you can favorite these internal pages.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Yup. Ctrl+D also works, but displays the bookmark dialog to the left rather than to the right. I'll post screenshots soon.
Blocks: 1388835
Severity: minor → normal
OS: Unspecified → All
Hardware: Unspecified → All
Summary: "Favourite" button is not displayed on about:settings → Bookmark star button is not displayed on about: pages
Looks like its intended by bug #1388835,
but maybe we should exclude Bookmark star button from this ban on all about: pages (except about:newtab & about:blank to prevent errors, if users will try to bookmark page too fast and URL it not loaded yet)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(adw)
I can understand why one would want not to bookmark about:blank or about:newpage; less so for settings, which I might want to access quickly and frequently (eg. to change proxy settings).
Note that even if we revert bug 1388835, which I think might make sense, about:home will be unaffected as it was taken care of independently in bug 1393802.
Whiteboard: [photon-structure][triage]
I assumed the suggestion came from UX, I just reviewed the implementation.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(abenson)
note that for about:home|blank|newtab we could even have a startup perf win if we don't show the star (AND we also properly make it not update its state, something we are not currently doing).
(In reply to Marco Bonardo [::mak] from comment #7)
> note that for about:home|blank|newtab we could even have a startup perf win
> if we don't show the star (AND we also properly make it not update its
> state, something we are not currently doing).

But as I hinted at in comment 13, this bug should not be concerned with these pages. about:newtab didn't have the star even before bug 1388835 and about:home is treated like about:newtab as of bug 1393802.
(In reply to Dão Gottwald [::dao] from comment #8)
> (In reply to Marco Bonardo [::mak] from comment #7)
> > note that for about:home|blank|newtab we could even have a startup perf win
> > if we don't show the star (AND we also properly make it not update its
> > state, something we are not currently doing).
> 
> But as I hinted at in comment 13

comment 5 even
While I can't imagine a lot of folks would do this, I can see a case for wanting to bookmark a preferences page so we should show the bookmark star but NOT the Page Action Menu.
Flags: needinfo?(abenson)
Flags: qe-verify?
Priority: -- → P4
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
Assignee: nobody → adw
Status: NEW → ASSIGNED
Flags: needinfo?(adw)
(In reply to Aaron Benson from comment #10)
> While I can't imagine a lot of folks would do this, I can see a case for
> wanting to bookmark a preferences page so we should show the bookmark star
> but NOT the Page Action Menu.

I'll revert the change for the bookmark star but not the other buttons then.
Iteration: --- → 57.3 - Sep 19
Priority: P4 → P1
(In reply to Drew Willcoxon :adw from comment #11)
> (In reply to Aaron Benson from comment #10)
> > While I can't imagine a lot of folks would do this, I can see a case for
> > wanting to bookmark a preferences page so we should show the bookmark star
> > but NOT the Page Action Menu.
> 
> I'll revert the change for the bookmark star but not the other buttons then.

Awesome, thank you very much!

I have small suggestion to patch,
like  Marco Bonardo [::mak] already wrote in Comment #7,
please exclude showing bookmark star button on:
- about:blank
- about:newtab

As there is no need to bookmark:
- about:blank - per its useless as bookmark (of course it could be accessible as about:newtab replacement by "+" (New Tab) button with extension)
- about:newtab - it's accessible by "+" (New Tab) button

This will prevent silly users errors, like mine in past - see bug #1371922,
where I wanted to bookmark some website page too fast with no URL loaded yet,
so in end I bookmarked about:blank/newtab by mistake.

I'm not fan of also banning about:home,
as user could have other not default Firefox page on Home button,
so to fast access about:home, bookmark placed in Bookmark Toolbar could be useful .
Aaron, one more q.  People in this bug (or one person at least) have suggested that about:home should show the bookmark star.  It did show the star until recently.  Dao has said in this bug (comment 5, comment 8) that bug 1393802 meant that the star should appear or not appear the same in both about:home and about:newtab, and since newtab doesn't show the star, home shouldn't either.  I looked through that bug and the referenced bug 1390359 and the Invision, but I don't see anything about that.  Should the star appear on about:home?
Flags: needinfo?(abenson)
(In reply to Drew Willcoxon :adw from comment #13)
> Should the star appear on about:home?

As Marco said in comment 7, not showing the star for about:home would be nice for startup performance, as it would let us delay Places initialization in the general case. In my opinion, the potential performance win outweighs the edge case mentioned in comment 12. Especially when the current plan is to have activity stream on about:home, making it equivalent to about:newtab from a user point of view.
Is there any data by how much Firefox is faster without showing bookmark star and other page actions buttons on pages like about:home?
(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #15)
> Is there any data by how much Firefox is faster without showing bookmark
> star and other page actions buttons on pages like about:home?

It's only about the first page that loads when starting the browser, not "pages like about:home". Showing the star requires having access to Places, which means loading the Places database off the disk, which can take more than a second (as any main thread IO, the time it takes is unpredictable, and potentially long if the disk is busy... which it often is during startup).
(In reply to Drew Willcoxon :adw from comment #13)
> Aaron, one more q.  People in this bug (or one person at least) have
> suggested that about:home should show the bookmark star.  It did show the
> star until recently.  Dao has said in this bug (comment 5, comment 8) that
> bug 1393802 meant that the star should appear or not appear the same in both
> about:home and about:newtab, and since newtab doesn't show the star, home
> shouldn't either.  I looked through that bug and the referenced bug 1390359
> and the Invision, but I don't see anything about that.  Should the star
> appear on about:home?

No star on about:home. From a UX perspective it doesn't make a whole lot of sense because there's the Home button for this.
Flags: needinfo?(abenson)
Which is to say, the home button provides a quick way back to this page aside from opening a new window/tab and bookmarking it is not needed/redundant.
This shows the star on all the about pages for which bug 1388835 hid it and the other action buttons.  It hides it on about:home, and the reason that works is because #urlbar[pageproxystate=invalid] now for about:home.
Comment on attachment 8905585 [details]
Bug 1396965 - Bookmark star button is not displayed on about: pages.

https://reviewboard.mozilla.org/r/177390/#review182400

Makes sense, thanks!
Attachment #8905585 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb2e73cf9473
Bookmark star button is not displayed on about: pages. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/bb2e73cf9473
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 57.0a1 (2017-09-08), so I'm marking this bug as VERIFIED. Thanks.
Status: RESOLVED → VERIFIED
QA Contact: Virtual
Flags: qe-verify?
You need to log in before you can comment on or make changes to this bug.