Closed Bug 1325880 Opened 3 years ago Closed 3 years ago

[RTL] When a link is close to Firefox's bottom, hovering its tooltip doesn't make the tooltip go to the left

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
firefox52 --- affected
firefox-esr52 --- affected
firefox53 --- verified
firefox54 --- verified
firefox55 --- verified

People

(Reporter: itiel_yn8, Assigned: Gijs)

References

Details

(Keywords: rtl)

Attachments

(5 files)

STR:
1. Download & install any RTL build of Firefox
2. Open a page where you know there's inside a link located *very* close to the browser's bottom right (in the attached screenshot it's about:newtab)
3. Hover that link very closly to the bottom right, so you'd also hover the URL tooltip
4. Instead of going to the left side of the browser, the tooltip goes only a few pixels left.

See attached screenshots.
The same STR works fine on LTR builds.
I've tried pinpointing the exact regression (though I'm not sure this is even a regression) and Firefox 39 also suffers from this bug (older versions may also have the same symptoms, didn't check it yet).
Attached file Testcase
Please note that the statusbar/tooltip border is broken when hovering the bottom right link from the testcase above.
Component: General → Layout: Text
Product: Firefox → Core
The issue can be observed also when loading a page and hovering the tooltip below (the one that says for example "Loading www.google.com").
See Also: → 1326404
Component: Layout: Text → Tabbed Browser
Product: Core → Firefox
(In reply to ItielMaN from comment #2)
> The same STR works fine on LTR builds.
> I've tried pinpointing the exact regression (though I'm not sure this is
> even a regression) and Firefox 39 also suffers from this bug (older versions
> may also have the same symptoms, didn't check it yet).

Is this a regression?
Flags: needinfo?(itiel_yn8)
(In reply to :Gijs (gone until 3 jan) from comment #6)
> (In reply to ItielMaN from comment #2)
> > The same STR works fine on LTR builds.
> > I've tried pinpointing the exact regression (though I'm not sure this is
> > even a regression) and Firefox 39 also suffers from this bug (older versions
> > may also have the same symptoms, didn't check it yet).
> 
> Is this a regression?

Can you tell me what version/build was this feature first implemented so I'd check?
Currently I'm blindly checking versions.
Flags: needinfo?(itiel_yn8)
(In reply to ItielMaN from comment #7)
> (In reply to :Gijs (gone until 3 jan) from comment #6)
> > (In reply to ItielMaN from comment #2)
> > > The same STR works fine on LTR builds.
> > > I've tried pinpointing the exact regression (though I'm not sure this is
> > > even a regression) and Firefox 39 also suffers from this bug (older versions
> > > may also have the same symptoms, didn't check it yet).
> > 
> > Is this a regression?
> 
> Can you tell me what version/build was this feature first implemented so I'd
> check?
> Currently I'm blindly checking versions.

bug 628654, Firefox 4.

It's totally possible this isn't a regression and has always been broken. I just don't know. If it *is* a regression, it'd be useful to know that.
Flags: needinfo?(itiel_yn8)
Executed command: ./mozregression --pref intl.uidirection.en-US:rtl --arg="https://bug1325880.bmoattachments.org/attachment.cgi?id=8821928"

9:21.03 INFO: Narrowed nightly regression window from [2013-09-11, 2013-09-13] (2 days) to [2013-09-12, 2013-09-13] (1 days) (~0 steps left)
 9:21.03 INFO: Got as far as we can go bisecting nightlies...
 9:21.03 INFO: Last good revision: a4e9c9c9dbf9 (2013-09-12)
 9:21.03 INFO: First bad revision: b9029b1de410 (2013-09-13)
 9:21.03 INFO: Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a4e9c9c9dbf9&tochange=b9029b1de410

I guess this is a regression of bug 821687.
Flags: needinfo?(itiel_yn8)
(In reply to Tomer Cohen :tomer from comment #9)
> Executed command: ./mozregression --pref intl.uidirection.en-US:rtl
> --arg="https://bug1325880.bmoattachments.org/attachment.cgi?id=8821928"
> 
> 9:21.03 INFO: Narrowed nightly regression window from [2013-09-11,
> 2013-09-13] (2 days) to [2013-09-12, 2013-09-13] (1 days) (~0 steps left)
>  9:21.03 INFO: Got as far as we can go bisecting nightlies...
>  9:21.03 INFO: Last good revision: a4e9c9c9dbf9 (2013-09-12)
>  9:21.03 INFO: First bad revision: b9029b1de410 (2013-09-13)
>  9:21.03 INFO: Pushlog:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=a4e9c9c9dbf9&tochange=b9029b1de410
> 
> I guess this is a regression of bug 821687.

That looks very plausible, yes. Thanks for figuring this out!
Blocks: 821687
Flags: needinfo?(gijskruitbosch+bugs)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
The offset-inline-start/end changes aren't strictly necessary, I suspect, but they simplify the selectors involved and IMO the behaviour is easier to reason about with these selectors than with the previous ones.
Comment on attachment 8845372 [details]
Bug 1325880 - fix RTL issue with statuspanel display,

https://reviewboard.mozilla.org/r/118578/#review120486

::: browser/base/content/browser.css:867
(Diff revision 1)
>    position: fixed;
>    margin-top: -3em;
>    max-width: calc(100% - 5px);
>    pointer-events: none;
> +  offset-inline-start: 0;
> +  offset-inline-end: auto;

The above additions shouldn't be necessary. (The statuspanel[mirrror] changes are fine.)
Attachment #8845372 - Flags: review?(dao+bmo) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bc67ce5424a2
fix RTL issue with statuspanel display, r=dao
https://hg.mozilla.org/mozilla-central/rev/bc67ce5424a2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Working great on latest Nightly.
Marking as VERIFIED FIXED.
Status: RESOLVED → VERIFIED
Comment on attachment 8845372 [details]
Bug 1325880 - fix RTL issue with statuspanel display,

Approval Request Comment
[User impact if declined]: target link doesn't display well on RTL builds
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes, verified by ItielMan
[Needs manual test from QE? If yes, steps to reproduce]: testcase included
[Is the change risky?]: No
[Why is the change risky/not risky?]: Minor CSS change
Attachment #8845372 - Flags: approval-mozilla-beta?
Attachment #8845372 - Flags: approval-mozilla-aurora?
Comment on attachment 8845372 [details]
Bug 1325880 - fix RTL issue with statuspanel display,

(In reply to Tomer Cohen :tomer from comment #18)
> Comment on attachment 8845372 [details]
> Bug 1325880 - fix RTL issue with statuspanel display,
> 
> Approval Request Comment
> [User impact if declined]: target link doesn't display well on RTL builds
> [Is this code covered by automated tests?]: No
> [Has the fix been verified in Nightly?]: Yes, verified by ItielMan
> [Needs manual test from QE? If yes, steps to reproduce]: testcase included
> [Is the change risky?]: No
> [Why is the change risky/not risky?]: Minor CSS change

Please don't request uplift on patches that you didn't write or review without checking with the author/reviewer, esp. if you're not super familiar with the code. If you think something deserves uplift, ask the author/reviewer (maybe with a needinfo).

As it is, I thought this request made sense but was lacking details. It seems you've just deleted some bits of the form? Here's an amended uplift request:


Approval Request Comment
[Feature/Bug causing the regression]: old regression in the behaviour of the status tooltips that replaced the status panel
[User impact if declined]: in RTL, links on the bottom-right-hand-side of the viewport can be obscured by the status panel when the user hovers over them (rather than the panel moving out of the way as it's supposed to). More generally, the status panel continues obscuring whatever is there if it appears based on keyboard focus (despite the position of the mouse)
[Is this code covered by automated tests?]: not to my knowledge, certainly not for this particularity.
[Has the fix been verified in Nightly?]: yes, see comment 
[Needs manual test from QE? If yes, steps to reproduce]:

comment #0 has one, but these might be a little more explicit rather than QE having to find a page with links in the bottom corner of the screen:

1. open Firefox on an RTL build (or use the "force RTL" add-on or flip the intl.uidirection pref (on recent builds))
2. open about:newtab
3. focus the search field
4. hit the tab key until you hit a link (ie tile)
5. move the mouse to the bottom right corner without hovering any other tiles

Expected: status tooltip with the URL moves out of the way
Pre-patch: the tooltip shifts a tiny bit but not to the other side of the viewport

[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: it's a minor CSS change that shouldn't impact anything besides this panel, where issues are quickly spotted.
[String changes made/needed]: no
Comment on attachment 8845372 [details]
Bug 1325880 - fix RTL issue with statuspanel display,

Fix RTL issue with statuspanel display and was verified. Beta53+ & Aurora54+.
Attachment #8845372 - Flags: approval-mozilla-beta?
Attachment #8845372 - Flags: approval-mozilla-beta+
Attachment #8845372 - Flags: approval-mozilla-aurora?
Attachment #8845372 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
Reproduced on Firefox 52 on Windows 10 x64 using the en-US build switched to RTL.

Confirming the fix on Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 14.04 x64 using the following Firefox builds:
* latest 54.0a2 Aurora, build ID: 20170314004020
* 53 beta 2, build ID: 20170313154936.
You need to log in before you can comment on or make changes to this bug.