Closed Bug 1194842 Opened 9 years ago Closed 9 years ago

[RTL] Unable to edit keyword in RTL locale

Categories

(Firefox :: Settings UI, defect)

Product:
Firefox
Component:
Settings UI
Version:
40 Branch
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 43
Tracking Flags:
Tracking Status
firefox40 --- affected
firefox41 + verified
firefox42 + fixed
firefox43 + verified
firefox-esr31 --- unaffected
firefox-esr38 - affected

People

(Reporter: alice0775, Assigned: ehsan.akhgari)

References

Details

(Keywords: regression, rtl)

Attachments

(2 files)

screenshot
100.57 KB, image/png
Details
Fix the width calculation of the textbox for editable XUL tree cells in RTL mode
1.16 KB, patch
jaws
: review+
ritu
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta+
Details | Diff | Splinter Review
Attached image screenshot — 
[Tracking Requested - why for this release]: Search keyword feature is broken due to regression

Steps to Reproduce:
1. Open RTL languages Firefox
2. Open about:preferences#search
3. Try to input/edit keyword

Actual Results:
Small input box appeared. But Unable to enter text.

Expected Results:
The input box should have a adequate width, should be able to enter text
ehsan, is this something you might look at?  Or can you suggest someone who may have time to fix this? A fairly minor regression.
Flags: needinfo?(ehsan)
Sure, I'll see what I can do.
Alice0775, if you resize the window (change window width e.g.), does the text box become editable so you can enter some text? I am trying to determine if there a way to workaround this issue or not. Thanks!
Flags: needinfo?(alice0775)
(In reply to Ritu Kothari (:ritu) from comment #4)
> Alice0775, if you resize the window (change window width e.g.), does the
> text box become editable so you can enter some text? I am trying to
> determine if there a way to workaround this issue or not. Thanks!

Nothing changed.
Flags: needinfo?(alice0775)
Component: Search → Preferences
Looks like I screwed up the width calculation in bug 140759...
Blocks: 140759
Flags: needinfo?(ehsan)
Comment on attachment 8657227 [details] [diff] [review]
Fix the width calculation of the textbox for editable XUL tree cells in RTL mode

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

::: toolkit/content/widgets/tree.xml
@@ +369,5 @@
>              // in LTR mode, and left side of the cell in RTL mode.
>              var left, widthdiff;
>              if (style.direction == "rtl") {
>                left = cellRect.x;
> +              widthdiff = cellRect.x - textRect.x;

I don't fully understand this. Is the X component always calculated from the top-left of the screen? That's what I see with:

data:text/html,<body dir=rtl><div id="d"style="background:yellow;position:absolute; left:100px; top:100px; width:100px; height:100px;"></div></body>

then running,
`document.getElementById("d").getBoxQuads()`

If that's the case, the widthdiff here for LTR is calculating the difference between the start of the cell and the start of the textbox. However for RTL, we would want to calculate cellRect.x + cellRect.width - textRect.x + textRect.width, which is apparently the same misunderstanding that you originally had.

Can you explain why that doesn't work?
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
Here is a sample cell in LTR:

+------------------------------------+
|                                    |
| Cell text                          |
|                                    |
+------------------------------------+
  ^..............textRect............^
^...............cellRect.............^

Here, textRect.x - cellRect.x gives you the number of pixels that you need to subtract from cellRect.width so that the textbox's left side aligns with the left side of the cell text.  Here is the same cell in RTL mode:

+------------------------------------+
|                                    |
|                          Cell text |
|                                    |
+------------------------------------+
  ^..............textRect............^
^...............cellRect.............^

I hope this picture makes it clear why cellRect.x + cellRect.width - textRect.x + textRect.width is wrong.

You may ask yourself why is it that we just use one code path for computing widthdiff?  The answer is that the width of the text rect is actually not computed exactly, and we guesstimate it based on the remaining width, whereas we compute our paddings properly(!), so in effect we would actually need to increase the width of the text box otherwise the rightmost text part (for example the "xt" in the RTL case above) would show from behind the textbox, so the RTL code path is designed to give you a negative number, hence increasing the width.

See nsTreeBodyFrame::GetCoordsForCellItem() for the broken way in which we compute these dimensions if you're curious about the details.
Flags: needinfo?(ehsan)
Comment on attachment 8657227 [details] [diff] [review]
Fix the width calculation of the textbox for editable XUL tree cells in RTL mode

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

Thanks, I didn't notice the part about this getting us a negative number causing the width to increase.
Attachment #8657227 - Flags: review?(jaws) → review+
Jared, would you be able to request uplift to Beta soon? We gtb Beta9 (last Beta) in 2 days. Thanks!
Flags: needinfo?(jaws)
Redirecting...
Flags: needinfo?(jaws) → needinfo?(ehsan)
Comment on attachment 8657227 [details] [diff] [review]
Fix the width calculation of the textbox for editable XUL tree cells in RTL mode

Approval Request Comment
[Feature/regressing bug #]: This is a bit complicated.  This is a bug in the XUL tree widget, which was technically introduced in bug 140759, since that is where that code was written, but before that bug XUL trees had no RTL support at all.  What has made this bug visible in the new in-content preferences which is the only place in the Firefox UI where we are using this code, so this has been broken in RTL locales since the search settings was moved to the in-content preferences in bug 1106559 since Firefox 35.
[User impact if declined]: See comment 0, the UI looks broken in RTL mode, and it makes editing search keyboards difficult.
[Describe test coverage new/current, TreeHerder]: Tested manually.
[Risks and why]: This should be very low risk.
[String/UUID change made/needed]: None.
Flags: needinfo?(ehsan)
Attachment #8657227 - Flags: approval-mozilla-beta?
Attachment #8657227 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/8758ef717d42
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Comment on attachment 8657227 [details] [diff] [review]
Fix the width calculation of the textbox for editable XUL tree cells in RTL mode

Patch seems simple, let's uplift to Beta41 and Aurora42.
Attachment #8657227 - Flags: approval-mozilla-beta?
Attachment #8657227 - Flags: approval-mozilla-beta+
Attachment #8657227 - Flags: approval-mozilla-aurora?
Attachment #8657227 - Flags: approval-mozilla-aurora+
Alice0775, could you please verify the fix works as expected on 09-10 Nightly or later? Thanks in advance.
Flags: needinfo?(alice0775)
Flags: qe-verify+
(In reply to Ritu Kothari (:ritu) from comment #17)
> Alice0775, could you please verify the fix works as expected on 09-10
> Nightly or later? Thanks in advance.

verified on Nightly43.0a1 Windows7.
https://hg.mozilla.org/mozilla-central/rev/dd2a1d737a64d9a3f23714ec5cc623ec8933b51f
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:43.0) Gecko/20100101 Firefox/43.0 ID:20150910030225
Flags: needinfo?(alice0775)
(In reply to Alice0775 White from comment #20)
> (In reply to Ritu Kothari (:ritu) from comment #17)
> > Alice0775, could you please verify the fix works as expected on 09-10
> > Nightly or later? Thanks in advance.
> 
> verified on Nightly43.0a1 Windows7.
> https://hg.mozilla.org/mozilla-central/rev/
> dd2a1d737a64d9a3f23714ec5cc623ec8933b51f
> Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:43.0) Gecko/20100101
> Firefox/43.0 ID:20150910030225

Awesome! Many Thanks.
Status: RESOLVED → VERIFIED
status-firefox43: fixed → verified
Flags: qe-verify+
Verified fixed using Firefox 41 Beta 9 (buildID: 20150910171927).
status-firefox41: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: