Closed Bug 1216096 Opened 9 years ago Closed 9 years ago

Backout of bug 1164693 - The directional caret should point in the opposite direction of what backspace will do

Categories

(Core :: Layout: Text and Fonts, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox42 --- wontfix
firefox43 --- fixed
firefox44 --- fixed
firefox45 --- fixed

People

(Reporter: Sylvestre, Assigned: smontagu)

References

Details

Attachments

(1 file)

The implementation of bug 1164693 caused the bug 1207761.
As it is a severe regression for RTL users, we are going to backout the implementation.
However, I am not sure how far we would like to backout the patch.
Simon, sorry to ask that but it seems that you are the best person to deal with that (it is not clear if Ted will work on this and we are very late in the 43 cycle).
Could you fill the uplift request for the backout ? (to do that by the book)
Thanks
I will deal with the sheriff about the "implementation".
Flags: needinfo?(smontagu)
This needs backout of the follow-up bugs bug 1177505 and 1180417 as well.

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=706a8c32356c
Flags: needinfo?(smontagu)
I think I can finish Bug 1207761 by the end of the week, if you're willing to wait a bit before landing this.
3 backout that late in the cycle, that seems too risky for 42... Maybe for 43. :/
We won't be doing it.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
I think we should still aim to do this backout for at least 44, if not 43. A new solution in bug 1207761 is going to take a while.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Assignee: nobody → smontagu
Attachment #8685406 - Flags: review?(jfkthame)
Attachment #8685406 - Flags: review?(jfkthame) → review+
https://hg.mozilla.org/mozilla-central/rev/a675fc80caa9
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Should this backout get uplifted to aurora or even beta?   Can you request uplift for it? 
This seems a bit large of a patch to uplift to beta this late in the cycle. Do you think it would break other work?
Flags: needinfo?(smontagu)
I was going to reply confidently that the patch is just a return to an earlier known-good state and risk was minimal, but when I tested that assumption I discovered that though it applies and builds in both Aurora and Beta, it causes a regression in Aurora -- the caret hook completely disappears.

Unfortunately I don't have the free time to debug this right now.

In Beta it seems to work OK, but the Aurora regression makes me nervous so I hesitate to request any uplift.
Flags: needinfo?(smontagu)
Better to know that now than after uplifting. Thanks for checking!
Blocks: 1221904
(In reply to Simon Montagu :smontagu from comment #11)
> I was going to reply confidently that the patch is just a return to an
> earlier known-good state and risk was minimal, but when I tested that
> assumption I discovered that though it applies and builds in both Aurora and
> Beta, it causes a regression in Aurora -- the caret hook completely
> disappears.

I just tried building mozilla-aurora with the backout patch, and the caret hook still works for me (provided I have bidi.browser.ui set to true). Is it possible something went wrong in your testing (maybe it was running with a fresh profile that didn't have that pref, or something?)
Flags: needinfo?(smontagu)
On what platform? The problem might well be OS- or version- specific. bidi.browser.ui was set to true in my tests, and I double-checked that reverting and applying the patch to and fro made the caret appear and disappear. Note that XUL input fields like the URL bar still worked, but the bug was visible in HTML content, whether <textarea>, <input> or any element with contenteditable=true.
Flags: needinfo?(smontagu)
(In reply to Simon Montagu :smontagu from comment #14)
> Note that XUL input fields like the URL bar still worked, but the
> bug was visible in HTML content, whether <textarea>, <input> or any element
> with contenteditable=true.

Ah, OK -- I was looking at XUL fields. Yes, in HTML content I see the problem.
(In reply to Simon Montagu :smontagu from comment #14)
> On what platform? The problem might well be OS- or version- specific.
> bidi.browser.ui was set to true in my tests, and I double-checked that
> reverting and applying the patch to and fro made the caret appear and
> disappear. Note that XUL input fields like the URL bar still worked, but the
> bug was visible in HTML content, whether <textarea>, <input> or any element
> with contenteditable=true.

Try with non-e10s mode, see bug 1191895.
(In reply to blinky from comment #16)
> Try with non-e10s mode, see bug 1191895.

Thank you! That fixes it.
Comment on attachment 8685406 [details] [diff] [review]
Combined backout patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1164693 
[User impact if declined]: The directional indicator on the caret in RTL languages behaves incorrectly (details in bug 1207761)
[Describe test coverage new/current, TreeHerder]: Baked on trunk since 2015-11-11
[Risks and why]: The patch returns to the status quo before bug 1164693, so risk should be minimal. The regression observed on aurora is an existing bug with bidi keyboard detection under e10s and can be worked around if necessary by turning off e10s.
[String/UUID change made/needed]: None
Attachment #8685406 - Flags: approval-mozilla-beta?
Attachment #8685406 - Flags: approval-mozilla-aurora?
Simon, I have the latest DevEd44 installed, and then I enabled e10s and the pref for bidi.browser.ui and I do see the bidirectional caret on my win8 laptop. Now, comment 11 is confusing because with the backout, if the bidi caret goes away in e10s mode, then it is a new regression and not an existing one. Please let me know whether that makes sense or not.
Flags: needinfo?(smontagu)
My question as well. Would this cause a new regression in 44 aurora (with e10s enabled, which is now the default on aurora)?  Is this also a problem in Nightly (45) with e10s enabled?
Yes, Nightly has the same problem if e10s is enabled.
Flags: needinfo?(smontagu)
Another consideration, using the bidirectional caret already requires setting a pref.
(In reply to Ritu Kothari (:ritu) from comment #19)
> Simon, I have the latest DevEd44 installed, and then I enabled e10s and the
> pref for bidi.browser.ui and I do see the bidirectional caret on my win8
> laptop. Now, comment 11 is confusing because with the backout, if the bidi
> caret goes away in e10s mode, then it is a new regression and not an
> existing one. Please let me know whether that makes sense or not.

It's not a new regression -- you would have seen the same issue on a build before bug 1164693 was checked in if you had enables e10s there. Before bug 1164693, the direction of the bidi caret depended on the active keyboard language, which was vulnerable to the e10s-specific bug 1033488. After it, and until this backout, the direction depended on the direction of the text next to the caret so that bug became less visible.
(In reply to Simon Montagu :smontagu from comment #24)
> (In reply to Ritu Kothari (:ritu) from comment #19)
> > Simon, I have the latest DevEd44 installed, and then I enabled e10s and the
> > pref for bidi.browser.ui and I do see the bidirectional caret on my win8
> > laptop. Now, comment 11 is confusing because with the backout, if the bidi
> > caret goes away in e10s mode, then it is a new regression and not an
> > existing one. Please let me know whether that makes sense or not.
> 
> It's not a new regression -- you would have seen the same issue on a build
> before bug 1164693 was checked in if you had enables e10s there. Before bug
> 1164693, the direction of the bidi caret depended on the active keyboard
> language, which was vulnerable to the e10s-specific bug 1033488. After it,
> and until this backout, the direction depended on the direction of the text
> next to the caret so that bug became less visible.

This makes sense. Thanks for providing the additional details. I have also ni'd jimm to consider fixing that bug before we enable e10s "by default" to a widespread beta population(in FF45 or later), see https://bugzilla.mozilla.org/show_bug.cgi?id=1033488#c6. Will approve the uplift to Aurora.
Comment on attachment 8685406 [details] [diff] [review]
Combined backout patch

This has been on Nightly for almost 3 weeks and fixes a sec issue as well. Taking it for Aurora and Beta.
Attachment #8685406 - Flags: approval-mozilla-beta?
Attachment #8685406 - Flags: approval-mozilla-beta+
Attachment #8685406 - Flags: approval-mozilla-aurora?
Attachment #8685406 - Flags: approval-mozilla-aurora+
Great News. Thank you all.
m-i a675fc80caa9	Simon Montagu — Bug 1216096: restore previous RTL caret behaviour by backout of bug 1164963, bug 1177505, and bug 1180417. r=jfkthame

m-aurora b4e23b2390f5	Simon Montagu — Bug 1216096: restore previous RTL caret behaviour by backout of bug 1164963, bug 1177505, and bug 1180417. r=jfkthame, a=rkothari tip 

m-beta 274a69572dda	Simon Montagu — Bug 1216096: restore previous RTL caret behaviour by backout of bug 1164963, bug 1177505, and bug 1180417. r=jfkthame, a=rkothari

It seems that the commit message is typo.  bug 1164963 should be bug 1164693, should not it?
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: