Open Bug 399850 Opened 15 years ago Updated 4 months ago

Find out if the bidi resolver doesn't need to split frames with rtl and ltr characters but the same embedding level

Categories

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

defect
Not set
normal

Tracking

()

People

(Reporter: smontagu, Unassigned)

References

(Depends on 2 open bugs)

Details

(Keywords: rtl)

Attachments

(3 files)

In the old gfx implementations we couldn't render a run of text containing characters with different native directions (equivalent to the charType frame property) but the same resolved direction (the embeddingLevel property) in a single operation, so the bidi resolver splits frames so that every frame contains text with the same native direction and resolved direction.

For example <span style="unicode-bidi: bidi-override; direction: ltr">world ELLOH</span> where "HELLO" is in a RTL language would be split into two frames.

I'm not 100% sure, but I think that with Thebes that's no longer necessary, and we could simplify code and maybe remove the charType property altogether.
Depends on: 402427
Of course, the presupposition behind this bug is "everyone uses Thebes". Is that true today? Is there a bug about disabling non-Thebes gfx implementations that this can depend on?
The only people who aren't using thebes are the Nokia MicroB team, and they already have a heavily patched build, so maybe they can patch around this too.
they can try to hack around anything.
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: layout.bidi → layout.fonts-and-text
Attached patch Patch part 1Splinter Review
This is the easy part, removing CalculateCharType from Resolve().

For the change to nsTextFrameThebes, see bug 393758 comment 2 and 3.
Attachment #443850 - Flags: review?(ehsan)
Comment on attachment 443850 [details] [diff] [review]
Patch part 1

The patch looks sane to me, but I can't say that I'm the most familiar person with this code, so could you ask for a peer review here as well?
Attachment #443850 - Flags: review?(ehsan) → review+
Attachment #443850 - Flags: review?(roc)
BTW, I checked in a bunch of reftests for this some time back, layout/reftests/bidi/mixedChartype*
Flags: in-testsuite+
http://hg.mozilla.org/mozilla-central/rev/5ecb4057a631

Keeping this open for possibly removing CalculateCharType from nsBidiPresUtils::ProcessText (which would mean that we could remove a whole lot more).
This caused a new assertion in our reftest runs:

###!!! ASSERTION: can't continue text run across non-fluid continuations: 'aFrame1->GetContent() != aFrame2->GetContent() || aFrame1->GetNextInFlow() == aFrame2', file /builds/slave/mozilla-central-linux-debug/build/layout/generic/nsTextFrameThebes.cpp, line 1338

fires twice on:

layout/generic/crashtests/472776-1.html
Simon, I guess we should turn that assertion back into a conditional check for now (partial backout, basically) while we figure out what's going on in that test.
Does that backout need review, or can it just be landed?
I was planning to just land it, but I wasn't online with a green tree today. I hope I'll get to it tomorrow.
(In reply to comment #10)
> This caused a new assertion in our reftest runs:
> 
> ###!!! ASSERTION: can't continue text run across non-fluid continuations:
> 'aFrame1->GetContent() != aFrame2->GetContent() || aFrame1->GetNextInFlow() ==
> aFrame2', file
> /builds/slave/mozilla-central-linux-debug/build/layout/generic/nsTextFrameThebes.cpp,
> line 1338
> 
> fires twice on:
> 
> layout/generic/crashtests/472776-1.html

... and also on:

layout/reftests/bidi/305643-1.html | assertion count 2
layout/reftests/bidi/492231-1.html | assertion count 1

(not sure why I didn't point those out the previous time)
So, there are two separate scenarios which cause the assertion:

1) When an element had bidirectional text, but changed leaving only single-direction text. During reflow after the change the bidi resolver leaves around the continuation frames that it created during initial reflow, but sets their length to zero.

2) In "visual order" pages we do something wildly inefficient: we do bidi resolution on the element and create continuation frames just like in logical order pages, but then set the embedding level property on each frame to the base paragraph level, so no reordering happens. This was probably necessary with the old text frame code where we had to flip visual text before passing it to the platform, but I don't think it's necessary now.

No. 2 would be quite easy to fix, if we care enough about visual pages to add a bit of extra code to handle them.

No. 1 I'm less sure about. Maybe we could make non-fluid continuations of zero-length frames into fluid continuations?

Or we could just leave things the way they are, though we might want at least to update the comment in ContinueTextRunAcrossFrames.
Couldn't the bidi resolver delete the zero-length continuations?
Hmm, I know we have been avoiding deleting frames during bidi resolution for performance reasons (see e.g. bug 319930 comment 7 and following), but possibly bug 512336 has removed those concerns?
hmm, that's a good point. Probably we should just leave things as-is.
Depends on: 570378
Depends on: 571185

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Assignee: smontagu → nobody
You need to log in before you can comment on or make changes to this bug.