Closed Bug 1472748 Opened 3 years ago Closed 3 years ago

Convert the listbox in "editBookmarkPanel.inc.xul" to "richlistbox"

Categories

(Firefox :: Bookmarks & History, enhancement, P1)

Product:
Firefox
Component:
Bookmarks & History
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file)

Bug 1472748 - Convert the listbox in "editBookmarkPanel.inc.xul" to "richlistbox".
59 bytes, text/x-review-board-request
mak
: review+
Details 
This is a slightly more complex case of bug 1472554.
I've requested review because the code is ready, but I'm defining some shared styles in bug 1472750 that we can reuse here.

Interestingly, while in the "listcell" element the "checked" attribute is inherited by the image and is used on Mac for styling, here it's sufficient to set the attribute on the "richlistitem", because this is what the native theming actually looks for:

https://searchfox.org/mozilla-central/rev/97d488a17a848ce3bebbfc83dc916cf20b88451c/widget/nsNativeTheme.cpp#208-210
Depends on: 1472750
Comment on attachment 8989387 [details]
Bug 1472748 - Convert the listbox in "editBookmarkPanel.inc.xul" to "richlistbox".

https://reviewboard.mozilla.org/r/254478/#review261430

::: browser/components/places/content/editBookmark.js:770
(Diff revision 1)
> -      elt.setAttribute("label", tag);
> +      let label = document.createElement("label");
> +      label.setAttribute("value", tag);
> +      elt.appendChild(label);
>        if (tagsInField.includes(tag))
>          elt.setAttribute("checked", "true");
>        tagsSelector.appendChild(elt);

while here could we append the tags to a document fragment and then append it into the RLB once?

::: browser/components/places/content/editBookmark.js:802
(Diff revision 1)
>        tagsSelectorRow.collapsed = false;
>        this._rebuildTagsSelectorList();
>  
>        // This is a no-op if we've added the listener.
> -      tagsSelector.addEventListener("CheckboxStateChange", this);
> +      tagsSelector.addEventListener("mousedown", this);
> +      tagsSelector.addEventListener("keypress", this);

Can we remove these when we collapse the selector?

::: browser/themes/linux/places/editBookmark.css:61
(Diff revision 1)
>   */
>  #editBMPanel_tagsField #treecolAutoCompleteValue {
>    visibility: collapse;
>  }
>  
> +#editBMPanel_tagsSelector > richlistitem > image {

the styling looks the same across platforms? You can problem put it into themes/shared/places/editBookmarkPanel.inc.css
Attachment #8989387 - Flags: review?(mak77)
Comment on attachment 8989387 [details]
Bug 1472748 - Convert the listbox in "editBookmarkPanel.inc.xul" to "richlistbox".

https://reviewboard.mozilla.org/r/254478/#review261834

it looks good in the Star panel, but it doesn't look good in the Library pane and in the Properties panel, at least on Windows 10 the checkbox image is squashed into a vertical line.
Attachment #8989387 - Flags: review?(mak77) → review-
Ah, I missed that this code was used there too. Well, the easiest solution is probably not to use the shared editBookmarkPanel.inc.css, since it's not loaded there, and there aren't many styles in the latest version anyways.
Comment on attachment 8989387 [details]
Bug 1472748 - Convert the listbox in "editBookmarkPanel.inc.xul" to "richlistbox".

https://reviewboard.mozilla.org/r/254478/#review261878

I see, it sounds like a pity we don't share that css, would you mind filing a bug in theme to better share it?
Attachment #8989387 - Flags: review?(mak77) → review+
Filed bug 1473597.
Comment on attachment 8989387 [details]
Bug 1472748 - Convert the listbox in "editBookmarkPanel.inc.xul" to "richlistbox".

It turns out that getIndexOfFirstVisibleRow doesn't always work correctly for the "richlistbox" element, making one of the tests fail. In fact, all the special code to preserve the scroll position is unneeded.

This is also the only consumer of this function apart from tests, and we can remove it when we remove the "listbox" element.
Attachment #8989387 - Flags: review+ → review?(mak77)
(In reply to :Paolo Amadini from comment #14)
> It turns out that getIndexOfFirstVisibleRow doesn't always work correctly
> for the "richlistbox" element, making one of the tests fail. In fact, all
> the special code to preserve the scroll position is unneeded.

That's not correct, if try these steps:
Add a bunch of tags to a bookmark, open the tags selector, scroll it down, close the tags selector, reopen the tags selector. The scrolling goes back to top.

Though, on the other side, this is the less critical use-case for that, so we can likely drop the behavior, there should be no compelling reason to keep moving between the text list and the selector, especially once we'll move the selector to a sub pane.
Comment on attachment 8989387 [details]
Bug 1472748 - Convert the listbox in "editBookmarkPanel.inc.xul" to "richlistbox".

https://reviewboard.mozilla.org/r/254478/#review263768
Attachment #8989387 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #15)
> That's not correct, if try these steps:
> Add a bunch of tags to a bookmark, open the tags selector, scroll it down,
> close the tags selector, reopen the tags selector. The scrolling goes back
> to top.
> 
> Though, on the other side, this is the less critical use-case for that, so
> we can likely drop the behavior, there should be no compelling reason to
> keep moving between the text list and the selector, especially once we'll
> move the selector to a sub pane.

Makes sense, I'll update the commit message with a note. Thanks for the quick review!
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72c622b851f3
Convert the listbox in "editBookmarkPanel.inc.xul" to "richlistbox". r=mak
https://hg.mozilla.org/mozilla-central/rev/72c622b851f3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Blocks: 1476639
You need to log in before you can comment on or make changes to this bug.