weirdness with layout and selection of mixed-direction text

RESOLVED FIXED in mozilla1.9beta5

Status

()

Core
Layout: Text
P1
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: Uri Bernstein (Google))

Tracking

({regression})

Trunk
mozilla1.9beta5
regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments)

(Reporter)

Description

10 years ago
Created attachment 310271 [details]
initial layout of the URL

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008031804 Minefield/3.0b5pre

Go to the URL specified, which contains some Arabic text. For me, when I first load the URL, the third paragraph is misdrawn next to the second paragraph, but the text is unselectable. Moving to a different tab and back makes everything draw correctly.

Select the text starting after the link, as shown in selection.png. Then click on the empty area. The missing text shows up. For me, I can then repeat the action: select the text, click on the third paragraph, and it disappears.

Apologies if this is hard to understand; I'm more than willing to clarify.
(Reporter)

Comment 1

10 years ago
Created attachment 310272 [details]
Weird selection
(Reporter)

Comment 2

10 years ago
Created attachment 310273 [details]
after clicking on second paragraph
(Assignee)

Comment 3

10 years ago
This regressed between 2008-02-16 and 2008-02-17. I strongly suspect bug 392785.

I can't create a testcase because I can't reproduce this with a local file.
Blocks: 392785
Flags: blocking1.9?
Keywords: regression
(Assignee)

Comment 4

10 years ago
Created attachment 310346 [details]
testcase

In this testcase, the word "English" should appear on its own line, below the word in Arabic.
The buggy behaviour ("English" initially appearing on the same line as the Arabic, selection weirdness later) can be triggered with this testcase in one of two ways:

1. append "#a" to the URL of the testcase, and the bug is immediately apparent. This is similar to the original bugzilla URL, and regressed, as I wrote above, on 2008-02-17.

2. Load the testcase with the regular URL (no #a), then click on the link at the top. "English" jumps up to the Arabic line. This regressed in late June 2007, probably due to the new tnsTextFrame (bug 367177).

So I suspect the real bug was introduced by bug 367177, and bug 392785 just caused it to be triggered in additional scenarios.
(Assignee)

Updated

10 years ago
Blocks: 367177
(Assignee)

Comment 5

10 years ago
Some further investigation suggests that the problem is that the second line doesn't get reflowed after bidi resolution, because it's not marked dirty by the bidi resolver, because (I think) lineNeedsUpdate isn't set when it should be.
(ref: bug 339699, bug 330373).

I'll try to continue investigating tomorrow. 
-> all/all

this is really layout issue.
OS: Mac OS X → All
Hardware: PC → All
(Assignee)

Comment 7

10 years ago
I compared what happens with the testcase (broken) with what happens in a similar case, in which the line right after the <a> element starts with LTR (hence-force: "the reference"). The reference does not exhibit the bug. 

In both cases, only the first line (the one with the <a> element) is marked dirty by Resolve().

In the reference case, the next line is reflowed, because it is marked dirty by nsBlockFrame::ReflowInlineFrame(). This happens because the newline textframe on the first line has its status is set to NS_FRAME_NOT_COMPLETE, in nsTextFrame::Reflow. This, in turn, happens because maxContentLength for this frame, as computed by nsTextFrame::GetInFlowContentLength(), is > 1.

In the testcase, nsTextFrame::GetInFlowContentLength() returns 1 (because there's a bidi continuation starting right after the newline), the line status is therefore set to NS_FRAME_COMPLETE, the next line isn't marked dirty, and is therefore not reflowed, and hence everything afterwards remains on the same line.

So, I'm still not sure where this should be fixed. Either Resolve() should mark the second line dirty, or the logic for setting the line's status to NS_FRAME_NOT_COMPLETE in nsTextFrame::Reflow should be changed. I tend to think it should be the former, but on the other hand, the bidi code as it is used to work fine with the old nsTextFrame.

Roc, dbaron - can you offer some help?
OS: All → Mac OS X
Hardware: All → PC
(Assignee)

Comment 8

10 years ago
Created attachment 310648 [details] [diff] [review]
patch

This fixes it, but might negate some (or most) of the performance benefit from bug 330373 (assuming there actually was benefit).

Requesting reviews from the reviewers of bug 330373, although if either of you (or dbaron) are comfortable with reviewing it alone, I don't mind, of course.
Assignee: nobody → uriber
Status: NEW → ASSIGNED
Attachment #310648 - Flags: superreview?(bzbarsky)
Attachment #310648 - Flags: review?(roc)
(Assignee)

Updated

10 years ago
Blocks: 330373
Comment on attachment 310648 [details] [diff] [review]
patch

I'm not comfortable reviewing this at all, if the review is needed in the next few weeks.  I just don't recall this code well enough.
(Assignee)

Comment 10

10 years ago
(In reply to comment #9)
> (From update of attachment 310648 [details] [diff] [review])
> I'm not comfortable reviewing this at all, if the review is needed in the next
> few weeks.  I just don't recall this code well enough.
> 

I doubt if there is anyone who recalls this code any better (you guided me through bug 330373), but perhaps roc will be able to help.
Attachment #310648 - Flags: review?(dbaron)
I'm kind of hoping he will; I'm just totally swamped right now...  Finding time to sort this out (esp. the perf implications) is very unlikely.
(Assignee)

Comment 12

10 years ago
Simon might also be able to help, of course.
Comment on attachment 310648 [details] [diff] [review]
patch

Looks good to me, but needs Simon
Attachment #310648 - Flags: superreview?(bzbarsky)
Attachment #310648 - Flags: superreview+
Attachment #310648 - Flags: review?(smontagu)
Attachment #310648 - Flags: review?(roc)
Attachment #310648 - Flags: review?(dbaron)
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Priority: P2 → P1
Whiteboard: [needs review smontagu]
Target Milestone: --- → mozilla1.9beta5
Comment on attachment 310648 [details] [diff] [review]
patch

Looks OK to me.
Attachment #310648 - Flags: review?(smontagu) → review+
Keywords: checkin-needed
Whiteboard: [needs review smontagu] → [reviewed patch in hand]
Attachment #310648 - Flags: approval1.9b5?

Comment 15

10 years ago
Comment on attachment 310648 [details] [diff] [review]
patch

a+ schrep
Attachment #310648 - Flags: approval1.9b5?
Attachment #310648 - Flags: approval1.9b5+
Attachment #310648 - Flags: approval1.9+
Checking in layout/base/nsBidiPresUtils.cpp;
/cvsroot/mozilla/layout/base/nsBidiPresUtils.cpp,v  <--  nsBidiPresUtils.cpp
new revision: 1.115; previous revision: 1.114
done
Checking in layout/base/nsBidiPresUtils.h;
/cvsroot/mozilla/layout/base/nsBidiPresUtils.h,v  <--  nsBidiPresUtils.h
new revision: 1.29; previous revision: 1.28
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [reviewed patch in hand]
OS: Mac OS X → All
Hardware: PC → All
(Assignee)

Comment 17

10 years ago
I need to make a reftest for this.
Flags: in-testsuite?
(In reply to comment #17)
> I need to make a reftest for this.

Note that after bug 421353, the selecting will not rise reflow. So, the selection changing is not a good trigger for testing the regression.
(Assignee)

Comment 19

10 years ago
I checked in a reftest that doesn't require focusing links, or underlines.
Flags: in-testsuite? → in-testsuite+

Updated

9 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: layout.bidi → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.