Closed Bug 1466806 Opened 6 years ago Closed 6 years ago

The "expand" buttons are displayed outside the "Role" and "Name" panel on RTL builds

Categories

(DevTools :: Accessibility Tools, defect)

defect
Not set
normal

Tracking

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 unaffected, firefox61 wontfix, firefox62 verified, firefox63 verified)

VERIFIED FIXED
Firefox 62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 --- wontfix
firefox62 --- verified
firefox63 --- verified

People

(Reporter: emilghitta, Assigned: yzen)

References

Details

Attachments

(3 files)

Attached image RTL.gif
[Affected versions]:
Firefox 62.0a1 (BuildId:20180604220149).
Firefox 61.0b10 (BuildId:20180530184300).

[Affected platforms]:
Windows 10 64bit.
macOS 10.13.
Ubuntu 16.04 64bit.

[Steps to reproduce]:
1. Launch Firefox with an RTL locale (ex:arabic).
2. Access any website.
3. Enable the Accessibility features.
4. Expand several elements from the "Role" and "Name" panel.
5. Scroll down inside the "Role" and "Name" panel.

[Expected result]:
The elements from the "Role" and "Name" panel are properly displayed.

[Actual result]:
The "expand" buttons are displayed outside the "Role" and "Name" panel.

[Regression range]:
I don't think that this is a regression.

[Additional information]:
For further information regarding this issue please observe the attached screencast.
Attached patch 1466806 patchSplinter Review
This issue is two-fold:
* bug 1313486 did not fix value cells within the tree view. (see array example for expected view). Additionally the unicode-bidi styling should only apply to the text parts of the cell (not the whole cell) to be able to position the exapnder arrow correctly.
* when rendering the thead as sticky (non-scrollable) as in the accessibility panel, thead needs to have a slightly higher z-index because of some weird rendering issue with expanders overflowing the thead (see gif in description).
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
Attachment #8985181 - Flags: review?(odvarko)
Product: Firefox → DevTools
Comment on attachment 8985181 [details] [diff] [review]
1466806 patch

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

Thanks for the patch!

I've been testing it on my machine (Win10, Firefox Nightly) and it nicely fixes the reported problem.

---

One note related to an existing layout issue, not caused by this patch:

1) If I expand the document item (in RTL) there is a little white gap is created at the right side of the Header (just after the 'Role' label). This gap is transparent, so I can see e.g. the blue selection if it's underneath.

2) If i close the document item (again in RTL) the gap disappears and now the 'Role' label is moved to the right and partially hidden.

Changing size of the browser window (triggers re-layout) fixes the problem.

See the attached screenshot.

Perhaps it can be fixed in a follow up?
(I can file it if you want me too)

Thanks,
Honza
Attachment #8985181 - Flags: review?(odvarko) → review+
(In reply to Jan Honza Odvarko [:Honza] from comment #2)
> Comment on attachment 8985181 [details] [diff] [review]
> 1466806 patch
> 
> Review of attachment 8985181 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch!
> 
> I've been testing it on my machine (Win10, Firefox Nightly) and it nicely
> fixes the reported problem.
> 
> ---
> 
> One note related to an existing layout issue, not caused by this patch:
> 
> 1) If I expand the document item (in RTL) there is a little white gap is
> created at the right side of the Header (just after the 'Role' label). This
> gap is transparent, so I can see e.g. the blue selection if it's underneath.
> 
> 2) If i close the document item (again in RTL) the gap disappears and now
> the 'Role' label is moved to the right and partially hidden.
> 
> Changing size of the browser window (triggers re-layout) fixes the problem.
> 
> See the attached screenshot.
> 
> Perhaps it can be fixed in a follow up?
> (I can file it if you want me too)
> 
> Thanks,
> Honza

This is a weird one. I was trying to inspect what happens in that case, it looks like the table is wider than the rows in thead and tbody, however the layout inspector shows the same width for all of them :/. If you don't mind, Jan, could you file it?
Flags: needinfo?(odvarko)
Also looks like the issue is there even on m-c without this patch.
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d7c028d3d53
fix RTL for tree view row expanders overlay and positioning. r=Honza
(In reply to Yura Zenevich [:yzen] from comment #4)
> This is a weird one. I was trying to inspect what happens in that case, it
> looks like the table is wider than the rows in thead and tbody, however the
> layout inspector shows the same width for all of them :/. If you don't mind,
> Jan, could you file it?
Sure thing, done: bug 1469598

Honza
Flags: needinfo?(odvarko)
https://hg.mozilla.org/mozilla-central/rev/8d7c028d3d53
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Flags: qe-verify+
Given that it's just some CSS tweaks, what are your thoughts about having this on the radar for a dot release ride-along?
Flags: needinfo?(yzenevich)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> Given that it's just some CSS tweaks, what are your thoughts about having
> this on the radar for a dot release ride-along?

It'd be useful especially for the shield study that is going to be making this panel a default that is not limited to specific locales.
Flags: needinfo?(yzenevich)
I have managed to reproduce this bug on affected builds: 61.0a1 (2018-06-05), 62.0a1 (2018-05-06), using the STR from comment 0. 

This is verified fixed on Nightly 63.0a1 (20180723100101) and Beta 62.0b9 (20180713213322) on the following OSes: Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.13.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Given where we are in the cycle, I think we should just let this ride with 62 at this point.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: