Closed
Bug 612230
Opened 14 years ago
Closed 14 years ago
The bookmarks toolbar is empty in RTL Firefox if even one bookmark overflows the width of the toolbar
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 4.0b8
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
People
(Reporter: smontagu, Assigned: asaf)
References
Details
(Keywords: regression, rtl)
Attachments
(1 file)
(deleted),
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
In RTL mode the bookmarks toolbar works fine as long as it fits into the window width with no overflow. As soon as any bookmark overflows the width, all the bookmarks disappear into the overflow pop-up.
Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=29bc8f25685d&tochange=014349e11696 which looks like bug 528884
This was reported on the Mozilla Israel forum: http://www.mozilla.org.il/board/viewtopic.php?f=9&t=9515 (link in Hebrew)
Reporter | ||
Updated•14 years ago
|
blocking2.0: --- → ?
Comment 1•14 years ago
|
||
tentatively assigning to Mano, feel free to opt-out if you don't have time available.
Assignee: nobody → mano
Blocks: 560198
Assignee | ||
Updated•14 years ago
|
Status: NEW → ASSIGNED
Comment 2•14 years ago
|
||
Blocking. The impression of all toolbar bookmarks being gone is pretty serious primary UI breakage.
blocking2.0: ? → final+
Assignee | ||
Comment 3•14 years ago
|
||
Whoever reviewed that should, well, review this patch too.
Attachment #490639 -
Flags: review?(mak77)
Comment 4•14 years ago
|
||
Comment on attachment 490639 [details] [diff] [review]
Patch
>diff -r 23dcbd0c286c browser/components/places/content/browserPlacesViews.js
>@@ -1049,19 +1050,20 @@
>
> _updateChevronTimerCallback: function PT__updateChevronTimerCallback() {
> let scrollRect = this._rootElt.getBoundingClientRect();
> let childOverflowed = false;
> for (let i = 0; i < this._rootElt.childNodes.length; i++) {
> let child = this._rootElt.childNodes[i];
> // Once a child overflows, all the next ones will.
> if (!childOverflowed) {
>- let childRect = child.getBoundingClientRect();
>- childOverflowed = this._isRTL ? (childRect.left < scrollRect.left)
>- : (childRect.right > scrollRect.right);
>+ let childRect = child.getBoundingClientRect();
this indentation change is probably a typo
thank you!
Attachment #490639 -
Flags: review?(mak77) → review+
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 5•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/a1e24da8518f
and the nit fix (mano, don't put checkin-needed if there's more fix needed!):
http://hg.mozilla.org/mozilla-central/rev/622d3a8f8ab1
the first check-in i forgot to set mano as user, so probably going to back this all out again and re-land. ugh.
Comment 6•14 years ago
|
||
And now I've totally screwed my tree up trying to backout these. I'm going to leave it in as-is. Mano, if you want me to change it, ping me and I'll figure it out.
Comment 7•14 years ago
|
||
In case you back this out and reland it: the second getComputedStyle argument is optional.
Keywords: checkin-needed
Assignee | ||
Comment 8•14 years ago
|
||
Thanks.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Target Milestone: --- → Firefox 4.0b8
You need to log in
before you can comment on or make changes to this bug.
Description
•