1014231 - Use an attribute instead of a class for the bookmark-item state of the home/star buttons (and/or get rid of it entirely) and stop persisting their class

Status: RESOLVED WORKSFORME

(Firefox :: Bookmarks & History, defect)

Description

[:Gijs (he/him)]

Because the class persistence on these buttons causes issues and makes things go sadfaces because other things can alter the classes (add-ons, other code) and don't expect this persistence. That and XUL persistence can be finnicky. So we should just use attributes that we know nobody else will mess with (e.g. bookmarkitem="true" or something)

(for background, see e.g. bug 972286)

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yes, that sounds like a good idea. What about mentoring this bug?

Comment 2

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #1)
> yes, that sounds like a good idea. What about mentoring this bug?

Sure.

This will involve updating all of the selectors in:

http://mxr.mozilla.org/mozilla-central/search?string=.bookmark-item

that also use '#home-button' or '#bookmarks-menu-button', to either not have their :not(#home-button) bits or to be using [bookmark-item=true] instead of .bookmark-item.

And updating all of these cases:

http://mxr.mozilla.org/mozilla-central/search?string=%22bookmark-item%22

to be using an attribute instead of className/classList.

and finally to adjust the persist attribute on the bookmarks-menu-button and home-button to persist the "bookmark-item" attribute instead of the "class" attribute. (this code is in browser/base/content/browser.xul )

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

what about just changing #home-button and #bookmarks-menu-button to use an attribute (and add a selector matching on that attribute along with .bookmark-item) and persist that attribute, so we can remove persist="class" and solve this bug without changing all of that css code? The patch would be much smaller/simpler and would still achieve the result we want (remove persist="class")

Comment 4

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #3)
> what about just changing #home-button and #bookmarks-menu-button to use an
> attribute (and add a selector matching on that attribute along with
> .bookmark-item) and persist that attribute, so we can remove persist="class"
> and solve this bug without changing all of that css code? The patch would be
> much smaller/simpler and would still achieve the result we want (remove
> persist="class")

So in case this wasn't clear, this is what I meant to suggest as well. We should *only* be updating this thing for the #home-button and #bookmarks-menu-button - not for all the other actual bookmark items. Which means that better MXR links for what to do are:

these selectors need updating:

http://mxr.mozilla.org/mozilla-central/search?string=.bookmark-item&find=&findi=&filter=^[^\0]*home-button[^\0]*%24&hitlimit=&tree=mozilla-central
http://mxr.mozilla.org/mozilla-central/search?string=.bookmark-item&find=&findi=&filter=^[^\0]*bookmarks-menu-button[^\0]*%24&hitlimit=&tree=mozilla-central

and from this list:

http://mxr.mozilla.org/mozilla-central/search?string=%22bookmark-item%22

only the invocations that are adjusting the bookmarks-menu-button and the home-button need updating. That means NOT ALL OF THEM, but you'll need to inspect the results and see whether it needs changing or not.

Comment 5

[viv1fi]

(In reply to :Gijs Kruitbosch from comment #4)

I'm interested in this bug and believe that I understand the refactorization outlined above, but I have a couple of questions:

(a): Is this a good "first bug"?
(b): What tests could I run to ensure that a patch doesn't break anything?

Comment 6

[Akshendra Pratap Singh]

Attachment: 1014231.patch

Comment 7

[Akshendra Pratap Singh]

Attachment: 1014321.patch

Comment 8

[:Gijs (he/him)]

Comment on attachment 8508478 [details] [diff] [review]
1014321.patch

Review of attachment 8508478 [details] [diff] [review]:
-----------------------------------------------------------------

This is on the right track, but this should add "bookmark-item" to the persist attribute for the home and bookmark button, and remove the "class" attribute from there.

Also, can you let me know if you have try server access? If not, I can push the patch to try for you.

::: browser/base/content/browser.css
@@ +253,5 @@
>  %endif
>  
> +#main-window[inFullscreen] #global-notificationbox,
> +#main-window[inFullscreen] #high-priority-global-notificationbox {
> +  visibility: collapse;

This hunk shouldn't be here, I think?

@@ +509,5 @@
>  toolbar[mode="text"] #bookmarks-menu-button > .toolbarbutton-menubutton-button > .toolbarbutton-icon {
>    display: -moz-box !important;
>  }
>  toolbar[mode="text"] #bookmarks-menu-button > .toolbarbutton-menubutton-button > .toolbarbutton-text,
> +toolbar[mode="full"] #bookmarks-menu-button[bookmark-item="true"] > .toolbarbutton-menubutton-button > .toolbarbutton-text {

Nit: let's use bookmark-item=true (without quotes) throughout.

::: browser/base/content/browser.js
@@ +4912,5 @@
>      if (homeButton)
>        homeButton.className = homeButton.parentNode.id == "PersonalToolbar"
>                                 || homeButton.parentNode.parentNode.id == "PersonalToolbar" ?
> +                             {homeButton.className.replace("toolbarbutton-1", ""); homeButton.setAttribute("bookmark-item","true")} :
> +                             {homeButton.setAttribute("bookmark-item", "false"); homeButton.className += "toolbarbutton-1";}; 

Hm, no. Let's rewrite this to use classList instead:

if (homeButton) {
  let onPersonalToolbar = homeButton.parentNode.id == "PersonalToolbar" ||
                          homeButton.parentNode.parentNode.id == "PersonalToolbar";
  homeButton.classList.toggle("toolbarbutton-1", !onPersonalToolbar);
  if (onPersonalToolbar) {
    homeButton.setAttribute("bookmark-item", "true");
  } else {
    homeButton.removeAttribute("bookmark-item");
  }
}

::: browser/themes/osx/browser.css
@@ +269,5 @@
>  #personal-bookmarks[cui-areatype="toolbar"] > #bookmarks-toolbar-placeholder > .toolbarbutton-text {
>    display: -moz-box !important; /* Force the display of the label for bookmarks */
>  }
>  
> +toolbarbutton.bookmark-item:not(.subviewbutton):hover {

This and friends will need careful visual checking, because removing the :not(#id) bit from the selector will decrease the specificity of the rule. I expect this will be OK, but we should keep it in mind. :-)

Comment 9

[Akshendra Pratap Singh]

> This is on the right track, but this should add "bookmark-item" to the
> persist attribute for the home and bookmark button, and remove the "class"
> attribute from there.
Help me here, I should use persist in this way
homeButton.persist("bookmark-item","true");
instead of setAttribute().

> Also, can you let me know if you have try server access? If not, I can push
> the patch to try for you.
I do not have try server access

Comment 10

[:Gijs (he/him)]

(In reply to Akshendra Pratap Singh from comment #9)
> > This is on the right track, but this should add "bookmark-item" to the
> > persist attribute for the home and bookmark button, and remove the "class"
> > attribute from there.
> Help me here, I should use persist in this way
> homeButton.persist("bookmark-item","true");
> instead of setAttribute().

No, you need to change this:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#801

and this:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#918

to use persist="bookmark-item" instead of persist="class".

Comment 11

[Akshendra Pratap Singh]

Attachment: 1014321.patch

Comment 12

[:Gijs (he/him)]

Comment on attachment 8508663 [details] [diff] [review]
1014321.patch

Review of attachment 8508663 [details] [diff] [review]:
-----------------------------------------------------------------

You also need to change:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#1239

So now we get to the difficult part here... we need to ensure the styling of the home and bookmarks button either remains the same or is improved.

The current patch doesn't quite do that, at least on OS X. I think we should make sure that the right icon is displayed (there should be 16x16 versions of all the icons involved here; windows seems to already be using that for the bookmarks menu button, at least) and we otherwise use the same button styling as we do on the nav-bar.

What platforms have you tested your patch on and/or do you have access to?

::: browser/base/content/browser.js
@@ +4909,5 @@
>    {
>      if (!homeButton)
>        homeButton = document.getElementById("home-button");
> +    if (homeButton) {
> +      let onPersonalToolbar = homeButton.parentNode.id == "PersonalToolbar" || homeButton.parentNode.parentNode.id == "PersonalToolbar";

Nit: put the second half on its own line, align the "homeButton" bit with the homeButton bit of the previous line

@@ +4913,5 @@
> +      let onPersonalToolbar = homeButton.parentNode.id == "PersonalToolbar" || homeButton.parentNode.parentNode.id == "PersonalToolbar";
> +      homeButton.classList.toggle("toolbarbutton-1", !onPersonalToolbar);
> +      if (onPersonalToolbar) {
> +        homeButton.setAttribute("bookmark-item", "true");
> +      } 

Nit: trailing whitespace

::: browser/components/customizableui/test/browser_940107_home_button_in_bookmarks_toolbar.js
@@ +11,5 @@
>    bookmarksToolbar.collapsed = false;
>  
>    let homeButton = document.getElementById("home-button");
>    ok(homeButton.classList.contains("toolbarbutton-1"), "Home Button should have toolbarbutton-1 when in the nav-bar");
> +  ok(homeButton.getAttribute("bookmark-item") != "true", "Home Button should not be displayed as a bookmarks item");

nit: isnot(homeButton.getAttribute("bookmark-item"), "true", ...);

and similar changes below to use is() and isnot() instead of ok...

::: browser/themes/linux/browser.css
@@ +115,5 @@
>    -moz-padding-start: 4px;
>    -moz-padding-end: 2px;
>  }
>  
> +.bookmark-item > .toolbarbutton-icon,

This used to match the bookmarks menu button, and now it doesn't anymore. Is that OK?

::: browser/themes/osx/browser.css
@@ +233,5 @@
>  }
>  
>  /* ----- BOOKMARK BUTTONS ----- */
>  
> +toolbarbutton.bookmark-item:not(.subviewbutton),

Likewise, this used to match the home button.

Comment 13

[Akshendra Pratap Singh]

> What platforms have you tested your patch on and/or do you have access to?
I am working on Linux and the icons seems okay when I run the browser.

> This used to match the bookmarks menu button, and now it doesn't anymore. Is that OK?
> Likewise, this used to match the home button.
So at all these places the bookmark-item attribute will be true. We can use that I suppose.

Comment 14

[Akshendra Pratap Singh]

Can you attach an image or something showing the problem?

Comment 15

[:Gijs (he/him)]

Attachment: Hover state on OS X (note: no star icon, both nav-bar and bookmarks bar hover states)

(In reply to Akshendra Pratap Singh from comment #13)
> > What platforms have you tested your patch on and/or do you have access to?
> I am working on Linux and the icons seems okay when I run the browser.

Do you have access to Windows and/or Mac to test on at all, or just Linux?

> > This used to match the bookmarks menu button, and now it doesn't anymore. Is that OK?
> > Likewise, this used to match the home button.
> So at all these places the bookmark-item attribute will be true. We can use
> that I suppose.

Where necessary, yup. Maybe we should just try to keep the styling the same for now if that's easier... it's a little tricky because I'm fairly sure some bits of it are not right at the minute anyway.

(In reply to Akshendra Pratap Singh from comment #14)
> Can you attach an image or something showing the problem?

Done for OS X. I've not looked at Windows. If you don't have access to either, it might make sense to leave that part of the styling to someone else that does?

Comment 16

[Akshendra Pratap Singh]

I don't have access to the other two platforms only Linux.

Comment 17

[:Gijs (he/him)]

I will try and help get this patch unstuck. Needinfo'ing so I don't forget, although it might be early January before I get to it (because of Christmas / New Year's holidays and so on).

Comment 18

[:Gijs (he/him)]

It looks like we'll be doing this in bug 1182102.