Closed
Bug 1472748
Opened 4 years ago
Closed 4 years ago
Convert the listbox in "editBookmarkPanel.inc.xul" to "richlistbox"
Categories
(Firefox :: Bookmarks & History, enhancement, P1)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: Paolo, Assigned: Paolo)
References
Details
Attachments
(1 file)
This is a slightly more complex case of bug 1472554.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•4 years ago
|
||
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 3•4 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 5•4 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 6•4 years ago
|
||
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 hidden (mozreview-request) |
Comment 8•4 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 9•4 years ago
|
||
Filed bug 1473597.
Assignee | ||
Comment 10•4 years ago
|
||
This needed some test changes, new tryserver build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=28446af62c2561998f30c08022c67b14f0b4b288
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f0c2a11dc2a4117c6a61241bf1d69699ad3e107
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•4 years ago
|
||
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)
Comment 15•4 years ago
|
||
(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 16•4 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 17•4 years ago
|
||
(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!
Comment 18•4 years ago
|
||
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
Comment 19•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/72c622b851f3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in
before you can comment on or make changes to this bug.
Description
•