Closed Bug 1007067 Opened 6 years ago Closed 6 years ago

Textarea doesn't redraw properly when adding a newline to break a line (with RTL text in sibling element)

Categories

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

29 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox29 --- wontfix
firefox30 - verified
firefox31 - verified
firefox32 - verified
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: kenneth.venken, Assigned: mats)

References

Details

(Keywords: regression, rtl, testcase)

Attachments

(5 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140421221237

Steps to reproduce:

I'm using Firefox 29 on a Mac OSX 10.9.2. Haven't tested it on other operating systems yet.

- Open the attachment in Firefox
- Place your cursor before "{insert newline before opening bracket}"
- Press Enter



Actual results:

The last line of the textarea isn't redrawn correctly. The text "{insert newline before" is missing.

The reason - i think - is the RTL text in the select option. 


Expected results:

The "{insert newline before" text should be visible on the last line.
I can reproduce this, looks like another example of bug 838580
Confirmed FF 29, Win 7 x64
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Depends on: 838580
Ever confirmed: true
OS: Mac OS X → All
Product: Firefox → Core
Hardware: x86 → All
If I move the mouse hovering the textarea for a while the text is refreshed eventually,
but there's no line-break on the second line and the text overflows.  It seems that the RTL
text in the sibling element affects the line-breaking and frame trees inside the textarea.
Component: Layout → Layout: Text
Keywords: testcase
Priority: -- → P4
Summary: Textarea doesn't redraw properly when adding a newline to break a line → Textarea doesn't redraw properly when adding a newline to break a line (with RTL text in sibling element)
Attached file Frame dumps
The frame tree at the top is for the original (broken) test.
Note that there are only two lines here compared to the frame tree
on the bottom which has three (for the second test).
Duplicate of this bug: 1008164
Fwiw, using the STR in bug 1008164, that bug doesn't occur in FF27.0.1 but
it does occur in FF28 on Linux64.  It would help to know which 28 Nightly
introduced the bug.  (it seems bug 838580 has an older regression range,
possibly going back to DLBI in FF18 (bug 539356))
(In reply to Mats Palmgren (:mats) from comment #4)
> Created attachment 8420542 [details]
> Same test, without the <select>
This testcase doesn't reproduce the problem, while the initial one does.
Last good revision: a475f94bb1b1 (2013-11-17)
First bad revision: beddd6d4bcdf (2013-11-18)
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a475f94bb1b1&tochange=beddd6d4bcdf

I couldn't find the inbound builds so old to bisect further.
Attachment #8420542 - Attachment description: Same test, without the <select> → Same test, without the <select> (This works!)
(In reply to Paul Silaghi, QA [:pauly] from comment #9)
> Last good revision: a475f94bb1b1 (2013-11-17)
> First bad revision: beddd6d4bcdf (2013-11-18)
> Pushlog:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=a475f94bb1b1&tochange=beddd6d4bcdf

Thanks Paul. In that range, I would guess bug 646359 or bug 936935.
It looks like bug 936935 is the culprit.  When I add back the
"currentLine->MarkDirty()" after the "frame->AdjustOffsetsForBidi"
that was removed in that bug, the problem does not occur anymore.
Blocks: 936935
(In reply to Mats Palmgren (:mats) from comment #11)
> It looks like bug 936935 is the culprit.  When I add back the
> "currentLine->MarkDirty()" after the "frame->AdjustOffsetsForBidi"
> that was removed in that bug, the problem does not occur anymore.

What does this look like in the wild, to users?  Do we have any input from our Beta population regarding this regression?  It's been on GA for a couple of releases now so doesn't appear to be a tracking-worthy issue with the current information provided.  If there's a possibility of a clean backout to a known-good state (of bug 936935) we can look at that for FF30 as we're in our second-to-last week of Beta.
Flags: needinfo?(tdowner)
(In reply to Lukas Blakk [:lsblakk] from comment #12)
> If there's a possibility of a clean backout
> to a known-good state (of bug 936935) we can look at that for FF30 as we're
> in our second-to-last week of Beta.

I think adding back the line I indicated would be an acceptable wallpaper.
It wouldn't regress bug 936935.  It means we're doing slightly more work
in some cases, but the performance impact should be negligible.
Lukas,
I haven't seen any feedback from users about this. It does seem like it would mainly affect non en-us though? In that case we have little insight into what those users say on a daily basis.
Flags: needinfo?(tdowner)
(In reply to Mats Palmgren (:mats) from comment #13)
> (In reply to Lukas Blakk [:lsblakk] from comment #12)
> > If there's a possibility of a clean backout
> > to a known-good state (of bug 936935) we can look at that for FF30 as we're
> > in our second-to-last week of Beta.
> 
> I think adding back the line I indicated would be an acceptable wallpaper.
> It wouldn't regress bug 936935.  It means we're doing slightly more work
> in some cases, but the performance impact should be negligible.

If you can get a patch landed, tested, and nominated before next Monday's beta we can take the uplift.
Comment on attachment 8427460 [details] [diff] [review]
wallpaper

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

r=me as wallpaper for beta, but we need a more focused fix on trunk. Either leave this open or file a new bug for that.
Attachment #8427460 - Flags: review?(smontagu) → review+
Tyler Downer, it is inappropriate for a developer to make such assumptions as "seems to mainly affect non EN-US" especially in regards to software for viewing content on a multi-language, international landscape such as the web. Please don't try and diminish the importance of this bug.

I reported bug 1008164, and I did so due to the bug often being blamed on my PHP software project.

Here's one user's encounter with the bug: http://tildehash.com/?article=announcing-hashover-1.0#c11
Depends on: 1015128
Filed bug 1015128 to followup with a real fix later.
Keywords: checkin-needed
Comment on attachment 8427460 [details] [diff] [review]
wallpaper

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 936935
User impact if declined: it's hard to edit textareas on pages with RTL text in some situations; the text isn't repainted properly so it looks like some lines of text disappeared.
Testing completed (on m-c, etc.): just landed on m-i, not in Nightly yet
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: none
Attachment #8427460 - Flags: approval-mozilla-beta?
Attachment #8427460 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/7c11daa90b1a
Assignee: nobody → matspal
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Attachment #8427460 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed 32.0a1 (2014-05-27), Win 7 x64
Status: RESOLVED → VERIFIED
Attachment #8427460 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I reproduced the issue with Nightly from April 2nd (BuildID: 20140402030201).
The issue no longer reproduces with: FF 30 Beta 8 (BuildID: 20140527133511), latest FF 31.0a2 Aurora (BuildID: 20140528004008), and latest FF 32.0a1 Nightly (BuildID: 20140527030202).

Verified on Win 7 x64, Mac OS X 10.9, Ubuntu 13.10 x86.
Attached patch reftestSplinter Review
Attachment #8430443 - Flags: review?(smontagu)
Comment on attachment 8430443 [details] [diff] [review]
reftest

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

Excellent! 

(I tried using exactly the same approach for bug 645119, but it still didn't work there)
Attachment #8430443 - Flags: review?(smontagu) → review+
Duplicate of this bug: 838580
You need to log in before you can comment on or make changes to this bug.