1177505 - Couple of extra pixels on the caret when composing a tweet

Status: VERIFIED FIXED

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

Description

[alex_mayorga]

Steps:
1 When replying to a tweet on twitter.com
2 move the caret around the composed message

Result:
There are a couple odd pixels on the top right of the caret, see http://screencast.com/t/vajjlAAtvQDR

Expected result:
There are no extra pixels on the caret

Comment 2

[Alice0775 White]

Attachment: testcase

Comment 3

[Ted Clancy [:tedders1]]

Attachment: bug-1177505-fix.patch

Comment 4

[Ted Clancy [:tedders1]]

Green treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51a360bb4b21

Comment 5

[Simon Montagu :smontagu]

Comment on attachment 8626731 [details] [diff] [review]
bug-1177505-fix.patch

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

I'm concerned that this might cause regressions when deleting RTL text from an LTR paragraph. I want to test it for a while before r+ing.

Comment 6

[Simon Montagu :smontagu]

Comment on attachment 8626731 [details] [diff] [review]
bug-1177505-fix.patch

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

In a LTR textarea, type "abc<space>אבג<space>123"

Expected and actual results visually from left to right: the three Latin characters followed by the three digits followed by the three Hebrew characters.

Select the Hebrew characters and type "def"

Expected results visually from left to right: abc def 123
Actual results visually from left to right: abc 123 fed

This happens because when the Hebrew characters are present a bidi continuation to the text frame is created and given RTL directionality. Then after the Hebrew characters are no longer there the text is all LTR so runCount = 1. Before this change, because frameCount was > 1, we didn't make the early return from ResolveParagraph so the continuation frame was converted to a fluid continuation and restored to LTR directionality. With this change we do make the early return without checking frameCount, so the continuation frame still acts as RTL even though it only contains LTR characters.

Comment 8

[Ted Clancy [:tedders1]]

Attachment: bug-1177505-fix.patch

Comment 9

[Ted Clancy [:tedders1]]

Green treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d3484095208

Comment 10

[Simon Montagu :smontagu]

Comment on attachment 8639097 [details] [diff] [review]
bug-1177505-fix.patch

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

::: layout/base/nsBidiPresUtils.cpp
@@ +741,4 @@
>        aBpd->mParagraphDepth == 0 && aBpd->GetDirection() == NSBIDI_LTR &&
>        aBpd->GetParaLevel() == 0 &&
>        frame0 != NS_BIDI_CONTROL_FRAME &&
>        !frame0->Properties().Get(nsIFrame::EmbeddingLevelProperty()) &&

You should probably add a null check for frame0 here, since you've removed the check on frameCount.

Comment 11

[Ted Clancy [:tedders1]]

Attachment: bug-1177505-fix.patch

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/efd26ea929a0

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/efd26ea929a0

Comment 14

[(no longer active)]

Ted, should we uplift this fix into beta so that we can fix it for 41?

Comment 15

[Ted Clancy [:tedders1]]

Yes, because Bug 1164693 is in Firefox 41, this should be in Firefox 41 too.

I'll apply for uplift to beta.

Comment 16

[Ted Clancy [:tedders1]]

Comment on attachment 8641282 [details] [diff] [review]
bug-1177505-fix.patch

Approval Request Comment

[Feature/regressing bug #]:
Bug 1164693

[User impact if declined]:
The directional caret sometimes appears (haphazardly) on non-bidi webpages, like Twitter. Non-bidi users are sometimes confused by the appearance of the directional caret and mistake it for a visual rendering bug.

[Describe test coverage new/current, TreeHerder]:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d3484095208

[Risks and why]: 
None foreseen.

[String/UUID change made/needed]:
None

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Alex, could you please verify the fix on a latest Nightly whenever you get a chance? Thanks.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8641282 [details] [diff] [review]
bug-1177505-fix.patch

This patch has been in Nightly for almost a month now. Let's uplift to Beta41.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c325c1a91cd6

Comment 20

[alex_mayorga]

(In reply to Ritu Kothari (:ritu) from comment #17)
> Alex, could you please verify the fix on a latest Nightly whenever you get a
> chance? Thanks.

¡Hola Ritu!

WFM on Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:43.0) Gecko/20100101 Firefox/43.0 ID:20150828030205 CSet: 87e23922be375985d0b1906ed5ba5f095f323a38

See https://twitter.com/alex_mayorga/status/637317321984356352

¡Gracias!

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to alex_mayorga from comment #20)
> (In reply to Ritu Kothari (:ritu) from comment #17)
> > Alex, could you please verify the fix on a latest Nightly whenever you get a
> > chance? Thanks.
> 
> ¡Hola Ritu!
> 
> WFM on Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:43.0) Gecko/20100101
> Firefox/43.0 ID:20150828030205 CSet: 87e23922be375985d0b1906ed5ba5f095f323a38
> 
> See https://twitter.com/alex_mayorga/status/637317321984356352
> 
> ¡Gracias!

Thank you! :)