Closed Bug 1177505 Opened 5 years ago Closed 5 years ago

Couple of extra pixels on the caret when composing a tweet

Categories

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

x86_64
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
FxOS-S2 (10Jul)
Iteration:
42.1 - Jul 13
Tracking Status
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: alex_mayorga, Assigned: tedders1)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files, 3 obsolete files)

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
Flags: needinfo?(tclancy)
Attached file testcase
Attachment #8626319 - Attachment is obsolete: true
Assignee: nobody → tclancy
Flags: needinfo?(tclancy)
Attached patch bug-1177505-fix.patch (obsolete) — Splinter Review
Attachment #8626731 - Flags: review?(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 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.
Attachment #8626731 - Flags: review?(smontagu) → review-
Iteration: --- → 42.1 - Jul 13
Whiteboard: [systemsfe]
Target Milestone: --- → FxOS-S2 (10Jul)
Duplicate of this bug: 1183577
Attached patch bug-1177505-fix.patch (obsolete) — Splinter Review
Attachment #8626731 - Attachment is obsolete: true
Attachment #8639097 - Flags: review?(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.
Attachment #8639097 - Flags: review?(smontagu) → review+
Attachment #8639097 - Attachment is obsolete: true
Keywords: checkin-needed
Ted, should we uplift this fix into beta so that we can fix it for 41?
Flags: needinfo?(tclancy)
Yes, because Bug 1164693 is in Firefox 41, this should be in Firefox 41 too.

I'll apply for uplift to beta.
Flags: needinfo?(tclancy)
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
Attachment #8641282 - Flags: approval-mozilla-beta?
Alex, could you please verify the fix on a latest Nightly whenever you get a chance? Thanks.
Flags: needinfo?(alex_mayorga)
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.
Attachment #8641282 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [good first verify]
(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!
Status: RESOLVED → VERIFIED
Flags: needinfo?(alex_mayorga)
(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! :)
You need to log in before you can comment on or make changes to this bug.