Couple of extra pixels on the caret when composing a tweet

VERIFIED FIXED in Firefox 41

Status

()

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: alex_mayorga, Assigned: tedders1)

Tracking

Trunk
FxOS-S2 (10Jul)
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(firefox41 fixed, firefox42 fixed)

Details

(Whiteboard: [systemsfe])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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)
Comment hidden (spam)

Comment 2

4 years ago
Posted file testcase
Attachment #8626319 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Assignee: nobody → tclancy
Flags: needinfo?(tclancy)
(Assignee)

Comment 3

4 years ago
Posted 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-
(Assignee)

Updated

4 years ago
Iteration: --- → 42.1 - Jul 13
Whiteboard: [systemsfe]
Target Milestone: --- → FxOS-S2 (10Jul)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1183577
(Assignee)

Comment 8

4 years ago
Posted 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+
(Assignee)

Comment 11

4 years ago
Attachment #8639097 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/efd26ea929a0
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Comment 14

4 years ago
Ted, should we uplift this fix into beta so that we can fix it for 41?
Flags: needinfo?(tclancy)
(Assignee)

Comment 15

4 years ago
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)
(Assignee)

Comment 16

4 years ago
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]
(Reporter)

Comment 20

4 years ago
(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.