bidi algorithm implementation inconsistent with msie (newlines in <pre>)

RESOLVED FIXED in mozilla5

Status

()

Core
Layout: Text
--
minor
RESOLVED FIXED
13 years ago
6 years ago

People

(Reporter: Noam Tamim, Assigned: smontagu)

Tracking

(Blocks: 5 bugs, {perf})

Trunk
mozilla5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(17 attachments, 9 obsolete attachments)

164 bytes, text/html
Details
342 bytes, image/png
Details
344 bytes, image/png
Details
169 bytes, text/html
Details
11.53 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.84 KB, patch
roc
: review+
Details | Diff | Splinter Review
20.95 KB, patch
roc
: review+
Details | Diff | Splinter Review
11.35 KB, patch
smontagu
: review+
Details | Diff | Splinter Review
16.99 KB, patch
roc
: review+
Details | Diff | Splinter Review
10.26 KB, patch
Details | Diff | Splinter Review
10.21 KB, patch
Details | Diff | Splinter Review
4.15 KB, patch
roc
: review+
Details | Diff | Splinter Review
12.03 KB, patch
roc
: review+
Details | Diff | Splinter Review
19.68 KB, image/png
Details
1.49 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.39 KB, patch
roc
: review+
Details | Diff | Splinter Review
11.72 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040913 Firefox/0.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040913 Firefox/0.10

In mozilla, cross-line bidi text is rendered differently that in MSIE (or other
Microsoft applications, for that matter). 
A testcase is worth a thounsand words, so I will not write too much, and my next
comment will have a testcase.
Anyway, I don't know if this is a mozilla bug or an MSIE bug. I don't have
enough knowledge of the bidi algorithm.
So - if any of you do have that knowledge, please - either confirm this as a
bug, or (more likely) drop it as a WONTFIX. I don't mean mozilla to be
bug-compatible with IE.
More info in the next comment. Can't I attach testcases to bug reports?

Reproducible: Always
Steps to Reproduce:
(Reporter)

Comment 1

13 years ago
Created attachment 161392 [details]
part 1 (html)

This is the html source. Look at the following two images:
msie.png: how the file looks in MSIE.
mozilla.png: how the file looks in Mozilla.
Look at the position of the point on the first line.
(Reporter)

Comment 2

13 years ago
Created attachment 161393 [details]
how the file looks in MSIE.
(Reporter)

Comment 3

13 years ago
Created attachment 161394 [details]
how the file looks in Mozilla.
(Assignee)

Comment 4

13 years ago
This problem is alluded to in a code comment at
http://lxr.mozilla.org/seamonkey/source/layout/base/src/nsBidiPresUtils.cpp#486
Status: UNCONFIRMED → NEW
Component: Browser-General → Layout: BiDi Hebrew & Arabic
Ever confirmed: true
Summary: bidi algorithm implementation inconsistent with msie → bidi algorithm implementation inconsistent with msie (newlines in <pre>)
(Reporter)

Comment 5

13 years ago
Created attachment 161455 [details]
a no-<pre> testcase


The bug (if it is really a bug; again, I don't know the bidi algorithm well
enough) is not special just to <pre> sections. It occurs EVERYWHERE - in normal
html (as in the newly attahced testcase) as well as <textarea>s and
Thunderbird/MailNews messages.
The question is really this: should a \n, or a <br>, break the flow of the
text, or should it be treated like a regular whitespace/punctuation char?
(Assignee)

Comment 6

13 years ago
<br> in non-preformatted text is discussed in bug 229367. The two cases are not
necessarily equivalent (so this is not a dupe).
Assignee: general → smontagu
Confirming on OS X => All/All
OS: Windows XP → All
Hardware: PC → All

Updated

11 years ago
Blocks: 330350
(Assignee)

Updated

11 years ago
Depends on: 332655

Updated

11 years ago
Blocks: 330461

Updated

11 years ago
No longer blocks: 330461

Updated

9 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: general → layout.fonts-and-text
(Assignee)

Updated

8 years ago
Blocks: 477495

Comment 8

8 years ago
According to comments in bug 477495, this can lead to severe performance issues on some pages.
Keywords: perf
This is tested in the second test of:
http://test.csswg.org/suites/css2.1/20101001/xhtml1/bidi-breaking-002.xht
(I think tests 3 and 4 are invalid.)
Blocks: 605520
(Assignee)

Updated

7 years ago
Blocks: 613154
(Assignee)

Updated

7 years ago
Duplicate of this bug: 607541
(Assignee)

Comment 11

7 years ago
Copied from bug 607541 comment 0:

In elements where line breaks are not collapsed, e.g. <textarea> and elements
with white-space:pre|pre-line|pre-wrap, line breaks should constitute UBA
paragraph breaks.

When a line break introduces a UBA paragraph break, the base direction of the
new UBA paragraph will be determined by the computed direction of the nearest
ancestor element whose bidi properties require its contents to be in a separate
UBA paragraph (or sequence of paragraphs), e.g. a block element or an element
directionally isolated by the ubi attribute (which is being proposed in a
separate bug). Furthermore, for every element on the path in between that
results in the creation of an embedding or override level, e.g. a <bdo> element
or any element with a dir attribute or a value other than "normal" for the
unicode-bidi CSS property, the correspondeng embedding or override level is
re-introduced at the start of the new UBA paragraph (to be closed at the end of
the element or the UBA paragraph, whichever comes first).

For more information, see:
 *
<http://www.w3.org/International/docs/html-bidi-requirements/#newline-as-separator>
 * <http://www.w3.org/Bugs/Public/show_bug.cgi?id=10812>
(Assignee)

Comment 12

7 years ago
Created attachment 493937 [details] [diff] [review]
Patch part 1: preliminary refactoring -- combine InitLogicalArray and CreateBlockBuffer
(Assignee)

Comment 13

7 years ago
Created attachment 493938 [details] [diff] [review]
Patch part 2: more refactoring -- spin ResolveParagraph out of Resolve
(Assignee)

Comment 14

7 years ago
Created attachment 493939 [details] [diff] [review]
Patch part 3: Call ResolveParagraph on encountering <br> or embedded blocks

This fixes bug 229367 and bug 613157
(Assignee)

Comment 15

7 years ago
Created attachment 493940 [details] [diff] [review]
Tests for bug 229367 and bug 613157

This is a simple test based on http://www.w3.org/Bugs/Public/attachment.cgi?id=925.

I'll add more testcases from bug 229367 and its many dupes.
(Assignee)

Updated

7 years ago
Attachment #493939 - Attachment description: Patch part 3: Call ResolveParagraph on encountering <br> or embedding blocks → Patch part 3: Call ResolveParagraph on encountering <br> or embedded blocks
(In reply to comment #9)
> http://test.csswg.org/suites/css2.1/20101001/xhtml1/bidi-breaking-002.xht
> (I think tests 3 and 4 are invalid.)

Tests 3 and 4 are for the behavior of PARAGRAPH SEPARATOR (U+2029) and LINE SEPARATOR (U+2028), respectively, in <pre>. HTML4 used to say something rather ambiguous about them (http://www.w3.org/TR/REC-html40/struct/text.html#whitespace), but HTML5 no longer says anything at all about them, thus leaving them in the purview of CSS, and since that does not say anything about them either, they should be treated according to their Unicode definition, i.e. to start a new line (whether in pre-formatted text or not), with a new bidi paragraph for U+2029 and no new bidi paragraph for U+2028.

Thus, I think that test 3 *is* valid. Unfortunately, test 4 does have a bug. While its comment says "line separator does not break bidi paragraph", it does not actually test this because its second line both starts and ends with an RTL character. Test 4 should thus be something like this:

  <div class="set"> 
    <div class="test"> 
      <div class="pre">
&rlm;&nbsp; + - &times; &divide; &#x05D0;&#x2028;&nbsp; + - &times; &divide; &#x05EA;</div> 
    </div> 
    <div class="control"> 
      <div><bdo dir="ltr">&#x05D0; &divide; &times; - + &nbsp;</bdo></div> 
      <div><bdo dir="ltr">&#x05EA; &divide; &times; - + &nbsp;</bdo></div> 
    </div> 
  </div>
Tests 3 and 4 are invalid for the reasons stated here:
http://lists.w3.org/Archives/Public/public-css-testsuite/2010Oct/0118.html
(Assignee)

Comment 18

7 years ago
Summary of where we are with this:

The patches so far attached pass on tryserver and fix the related bugs bug 229367 and bug 613157 but don't address newlines in preformatted text, the actual issue that this bug is about.

I made a naïve patch to fix preformatted text like this:

   if (frame->GetStyleContext()->GetStyleText()->NewlineIsSignificant() &&
       frame->HasTerminalNewline()) {
     ResolveParagraph(aBlockFrame, PR_TRUE);

but it doesn't work at all well, I think for two main reasons:

* We do bidi resolution before line breaking, so the test for frame->HasTerminalNewLine() isn't right.
* Even assuming a solution to that, splitting the paragraph inside a content node breaks various assumptions in ResolveParagraph().
Blocks: 229367, 613157
Presumably instead of using frame->HasTerminalNewline, you need to test NewlineIsSignificant then actually scan the text looking for all \ns.
(Assignee)

Comment 20

7 years ago
Will it be OK to actually split the frames after the \ns, or will that confuse the line breaking code in nsTextFrame (or wherever)? If not, bidi resolution will have to pick up again for the next paragraph in mid-frame, which will be a significant complication.
It will be OK to break the frames after the \ns.
(Assignee)

Comment 22

7 years ago
Created attachment 495867 [details] [diff] [review]
Patch part 1 v2: preliminary refactoring -- combine InitLogicalArray and CreateBlockBuffer
Attachment #493937 - Attachment is obsolete: true
Attachment #495867 - Flags: review?(roc)
(Assignee)

Comment 23

7 years ago
Created attachment 495868 [details] [diff] [review]
Patch part 2 v2: more refactoring -- spin ResolveParagraph out of Resolve
Attachment #493938 - Attachment is obsolete: true
Attachment #495868 - Flags: review?(roc)
(Assignee)

Comment 24

7 years ago
Created attachment 495869 [details] [diff] [review]
Patch part 3 v2: Call ResolveParagraph on encountering <br> or embedding blocks
Attachment #493939 - Attachment is obsolete: true
Attachment #495869 - Flags: review?(roc)
(Assignee)

Comment 25

7 years ago
Created attachment 495870 [details] [diff] [review]
Patch part 4: call ResolveParagraph on encountering newlines in preformatted elements
Attachment #495870 - Flags: review?(roc)
(Assignee)

Comment 26

7 years ago
Created attachment 495873 [details] [diff] [review]
Patch part 5: optimize by not doing unnecessary resolution

Also removed some dead code.

This is a very significant optimization for huge text files with little bits of bidi, such as tinderbox logs.
Attachment #495873 - Flags: review?(roc)
(Assignee)

Comment 27

7 years ago
Created attachment 495874 [details] [diff] [review]
Tests for this bug

Again, this is a very basic test and I will add more.
Attachment #495867 - Flags: review?(roc) → review+
Comment on attachment 495868 [details] [diff] [review]
Patch part 2 v2: more refactoring -- spin ResolveParagraph out of Resolve

+  nsBlockInFlowLineIterator* mLineIter;

nsAutoPtr, so you can remove the manual delete
Attachment #495868 - Flags: review?(roc) → review+
+        if (frame->GetStyleContext()->GetStyleDisplay()->mDisplay !=
+            NS_STYLE_DISPLAY_INLINE) {

Is this correct? Should a display:inline-block really end the paragraph? I wouldn't have thought so. Seems to me you probably want IsInlineOutside().

I think mEmbeddingStack would be slightly clearer if it was an nsTArray<PRUnichar>. It shouldn't be interpreted as a string.

The flow of control in part 3 here seems a little weird to me. Before we did InitLogicalArray followed by ResolveParagraph which resolves the whole block. Now, during InitLogicalArray we call ResolveParagraph for all paragraphs except the last one, which has ResolveParagraph called by Resolve.

I think this would make more sense if you renamed InitLogicalArray to TraverseFrames and added a comment to the call to ResolveParagraph from Resolve to say "// Resolve final paragraph".
Attachment #495873 - Flags: review?(roc) → review+
+            nsIFrame* lastPreformattedFrame = frame;

This initializer is redundant.

+                /*
+                 * If there is no newline in the frame, just save the text and
+                 * do bidi resolution later
+                 */
+                mBuffer.Append(Substring(text, start));
+                break;

Don't we need to set lastPreformattedFrame here? I guess I'm not really sure what lastPreformattedFrame is supposed to mean. Is it supposed to be firstPreformattedFrame->GetLastContinuation()?

+              PRUint32 lineLength = endLine;

lineLength is a misnomer. Why not just keep using endLine?

+                if (!next) {
+                  // If the frame has no next in flow, create one
+                  CreateContinuation(frame, &next, PR_TRUE);
+                }

Why not move this up inside the previous 'if'?
(Assignee)

Comment 31

7 years ago
Created attachment 498344 [details] [diff] [review]
Patch part 1.5 -- get rid of nsDirectionalFrame

After part 1 stopped storing the bidi control codes in temporary frames, we can do away with the temporary frames altogether and not waste time creating and destroying them.
Attachment #498344 - Flags: review?(roc)
(Assignee)

Comment 32

7 years ago
Created attachment 498345 [details] [diff] [review]
Patch part 2 v3: more refactoring -- spin ResolveParagraph out of Resolve; updated to comments

transferring r=roc
Attachment #495868 - Attachment is obsolete: true
Attachment #498345 - Flags: review+
(Assignee)

Comment 33

7 years ago
Created attachment 498346 [details] [diff] [review]
Patch part 3 v3: Call ResolveParagraph on encountering <br> or embedding blocks; updated to comments

(In reply to comment #29)
> Is this correct? Should a display:inline-block really end the paragraph? I
> wouldn't have thought so. Seems to me you probably want IsInlineOutside().

You're right. Fixed and added a test for that case.

Also handled the logic for getting the right next sibling in TraverseFrames here instead of the lastPreformattedFrame stuff in [the previous version of] the next patch.
Attachment #495869 - Attachment is obsolete: true
Attachment #498346 - Flags: review?(roc)
Attachment #495869 - Flags: review?(roc)
(Assignee)

Comment 34

7 years ago
Created attachment 498347 [details] [diff] [review]
Patch part 4 v2: call ResolveParagraph on encountering newlines in preformatted elements; updated to comments
Attachment #495870 - Attachment is obsolete: true
Attachment #498347 - Flags: review?(roc)
Attachment #495870 - Flags: review?(roc)
(Assignee)

Comment 35

7 years ago
Created attachment 498349 [details] [diff] [review]
Tests for bug 229367 and bug 613157
Attachment #493940 - Attachment is obsolete: true
(Assignee)

Comment 36

7 years ago
Created attachment 498351 [details] [diff] [review]
Tests for this bug
Attachment #495874 - Attachment is obsolete: true
(Assignee)

Comment 37

7 years ago
Created attachment 498356 [details] [diff] [review]
printfs for debugging

I've had this in my local tree for years, and I'd like to check it in to save mq churn.
Attachment #498356 - Flags: review?(roc)
(Assignee)

Comment 38

7 years ago
Comment on attachment 498346 [details] [diff] [review]
Patch part 3 v3: Call ResolveParagraph on encountering <br> or embedding blocks; updated to comments

Sorry, there's a mistake in this patch
Attachment #498346 - Flags: review?(roc)
(Assignee)

Comment 39

7 years ago
Created attachment 498455 [details] [diff] [review]
Patch part 3 v4: Call ResolveParagraph on encountering <br> or embedding blocks
Attachment #498346 - Attachment is obsolete: true
Attachment #498455 - Flags: review?(roc)
Attachment #498344 - Flags: review?(roc) → review+
Attachment #498455 - Flags: review?(roc) → review+
Attachment #498356 - Flags: review?(roc) → review+
Attachment #498347 - Flags: review?(roc) → review+
(Assignee)

Comment 40

7 years ago
Comment on attachment 495873 [details] [diff] [review]
Patch part 5: optimize by not doing unnecessary resolution

Asking approval for this set of patches. It's not risk-free: I've tested extensively and fixed a number of regressions, but there is some chance that there will be other regressions.

I think it's worth taking the patches both for standards conformance and for performance: a tinderbox log with 85000+ lines loads for me in 7 seconds with these patches applied, as opposed to 80 seconds in an unpatched build.
Attachment #495873 - Flags: approval2.0?
Comment on attachment 495873 [details] [diff] [review]
Patch part 5: optimize by not doing unnecessary resolution

Tempting as it is, I think this isn't a good time to take it. Let's land it for post-FF4.
Attachment #495873 - Flags: approval2.0? → approval2.0-
(Assignee)

Updated

7 years ago
Depends on: 610267
(Assignee)

Updated

6 years ago
Blocks: 566066
Simon, please land the patches which are meant to be landed here on cedar.
(Assignee)

Comment 43

6 years ago
http://hg.mozilla.org/projects/cedar/rev/24402af6330a
http://hg.mozilla.org/projects/cedar/rev/b218c8609794
http://hg.mozilla.org/projects/cedar/rev/a3fe678d8560
http://hg.mozilla.org/projects/cedar/rev/12faff7e29ea
http://hg.mozilla.org/projects/cedar/rev/314216c47e6a
http://hg.mozilla.org/projects/cedar/rev/c926f46f97ce
http://hg.mozilla.org/projects/cedar/rev/6c2c72be33e0
http://hg.mozilla.org/projects/cedar/rev/56cc287f3860
http://hg.mozilla.org/projects/cedar/rev/91604d53c7ce
Whiteboard: [fixed-in-cedar]
http://hg.mozilla.org/mozilla-central/rev/24402af6330a
http://hg.mozilla.org/mozilla-central/rev/b218c8609794
http://hg.mozilla.org/mozilla-central/rev/a3fe678d8560
http://hg.mozilla.org/mozilla-central/rev/12faff7e29ea
http://hg.mozilla.org/mozilla-central/rev/314216c47e6a
http://hg.mozilla.org/mozilla-central/rev/c926f46f97ce
http://hg.mozilla.org/mozilla-central/rev/6c2c72be33e0
http://hg.mozilla.org/mozilla-central/rev/56cc287f3860
http://hg.mozilla.org/mozilla-central/rev/91604d53c7ce
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]
Target Milestone: --- → mozilla2.2
(Assignee)

Updated

6 years ago
Duplicate of this bug: 644768
Depends on: 645119
I backed this out because it caused bug 645119.  Sorry.  :(

http://hg.mozilla.org/mozilla-central/rev/0dc92bb949bf

Another thing to watch for is a bug which a Persian user reported to me.  The bidi resolution breaks when pressing a newline and waiting for a repaint.  I'll attach the screenshot that I received from this user next.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 521980 [details]
Screenshot of the bidi resolution problem on Persian wikipedia
(Assignee)

Updated

6 years ago
Depends on: 645193
Whiteboard: [not-ready-for-cedar]
(Assignee)

Comment 48

6 years ago
Created attachment 522787 [details] [diff] [review]
Fix for the regression described in comments 46-47
Attachment #522787 - Flags: review?(roc)
(Assignee)

Comment 49

6 years ago
Created attachment 522788 [details] [diff] [review]
Test for the regression
Attachment #522788 - Flags: review?(roc)
(Assignee)

Comment 50

6 years ago
Bug 645119 is caused by attachment 495873 [details] [diff] [review], which was a ride-along optimization and not really part of the fix for this bug. I'll open a new bug for it and fix the regression there.
Attachment #522788 - Flags: review?(roc) → review+
Attachment #522787 - Flags: review?(roc) → review+
(Assignee)

Updated

6 years ago
Whiteboard: [not-ready-for-cedar]
(Assignee)

Comment 51

6 years ago
http://hg.mozilla.org/projects/cedar/rev/f79edc2f8714
http://hg.mozilla.org/projects/cedar/rev/23b93110ab64
http://hg.mozilla.org/projects/cedar/rev/e6e29229954b
http://hg.mozilla.org/projects/cedar/rev/20aa82d70be2
http://hg.mozilla.org/projects/cedar/rev/d8aa1caf822d
http://hg.mozilla.org/projects/cedar/rev/f5ba8b30e673
http://hg.mozilla.org/projects/cedar/rev/71c1a795a43f
http://hg.mozilla.org/projects/cedar/rev/85f334fb73b1
http://hg.mozilla.org/projects/cedar/rev/cde1da5d8a8a
http://hg.mozilla.org/projects/cedar/rev/1e2727bc8036
http://hg.mozilla.org/projects/cedar/rev/2ad43389d244
Whiteboard: [fixed-in-cedar]
http://hg.mozilla.org/mozilla-central/rev/f79edc2f8714
http://hg.mozilla.org/mozilla-central/rev/e6e29229954b
http://hg.mozilla.org/mozilla-central/rev/20aa82d70be2
http://hg.mozilla.org/mozilla-central/rev/d8aa1caf822d
http://hg.mozilla.org/mozilla-central/rev/71c1a795a43f
http://hg.mozilla.org/mozilla-central/rev/85f334fb73b1
http://hg.mozilla.org/mozilla-central/rev/cde1da5d8a8a
http://hg.mozilla.org/mozilla-central/rev/2ad43389d244
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]
Depends on: 650189

Updated

6 years ago
Depends on: 650475
(Assignee)

Comment 53

6 years ago
Backed out again in http://hg.mozilla.org/mozilla-central/rev/fa0295a97f1b because of bug 650189
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 54

6 years ago
I've created a series of tryserver builds to narrow down which patch caused bug 650189:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smontagu@mozilla.com-9072c5d3dfcc
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smontagu@mozilla.com-e794a0c595ef
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smontagu@mozilla.com-ceebcc71f7a8
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smontagu@mozilla.com-681666d3bb11
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smontagu@mozilla.com-639aa83e113f

Tryserver seems to be rather backed up at the moment, but doubtless the builds will appear sooner or later.
Assignee: smontagu → nobody
Status: REOPENED → ASSIGNED
(Assignee)

Updated

6 years ago
Assignee: nobody → smontagu
OK, I tested bug 650189 with all of these builds, here are the results:

> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smontagu@mozilla.com-9072c5d3dfcc

No perf regression out of 10 runs.

> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smontagu@mozilla.com-e794a0c595ef

No perf regression (1 run out of 10 runs performed horribly, but I'm not sure what the cause was).

> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smontagu@mozilla.com-ceebcc71f7a8

Horrible perf when loading from network, good perf when loading from cache.  The only way that I can explain this is that this build might have the regression, but the regression _might_ be dependent on the way the data is fed into the reflow process.  By the same token, e794a0c595ef _might_ contain the regression as well (but I can't verify this claim).  I tested e794a0c595ef a lot of times after testing this builds, and couldn't reproduce the regression, so it might have been something else.

> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smontagu@mozilla.com-681666d3bb11

Same as ceebcc71f7a8.

> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smontagu@mozilla.com-639aa83e113f

Same as ceebcc71f7a8.
(Assignee)

Comment 56

6 years ago
How is http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smontagu@mozilla.com-4d67e3300692 ?
(In reply to comment #56)
> How is
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smontagu@mozilla.com-4d67e3300692
> ?

At first it seemed to be better, but I managed to reproduce the bug after a couple of tries.

Updated

6 years ago
Blocks: 582181
Does this bug cover the bidi paragraph separation for <br>, as specified in <http://www.whatwg.org/specs/web-apps/current-work/multipage/text-level-semantics.html#the-br-element>?
(Assignee)

Comment 59

6 years ago
That depends what you mean by "this bug". It's really bug 229367, but it's fixed by the patches in this bug, specifically attachment 498455 [details] [diff] [review]
(Assignee)

Comment 60

6 years ago
Created attachment 537515 [details] [diff] [review]
Patch part 2.5: cache the lines for each frame while building mLogicalArray

This patch kills two birds with one stone: it fixes the performance regression (bug 650189) and also prevents the regression in bug 650489.
Attachment #537515 - Flags: review?(roc)
Comment on attachment 537515 [details] [diff] [review]
Patch part 2.5: cache the lines for each frame while building mLogicalArray

Review of attachment 537515 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #537515 - Flags: review?(roc) → review+
(Assignee)

Comment 62

6 years ago
http://hg.mozilla.org/mozilla-central/rev/f2072cae59b5
http://hg.mozilla.org/mozilla-central/rev/5c8934933381
http://hg.mozilla.org/mozilla-central/rev/dc74911b2689
http://hg.mozilla.org/mozilla-central/rev/afb2e4dc86c8
http://hg.mozilla.org/mozilla-central/rev/a2450f7a0618
http://hg.mozilla.org/mozilla-central/rev/77f6221f3ed7
http://hg.mozilla.org/mozilla-central/rev/007a04736be8
http://hg.mozilla.org/mozilla-central/rev/07c3c99413cd
http://hg.mozilla.org/mozilla-central/rev/05de55d44dfe
http://hg.mozilla.org/mozilla-central/rev/3d475b322365
(Assignee)

Updated

6 years ago
Blocks: 650489
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 663295

Updated

6 years ago
Depends on: 663662
(Assignee)

Updated

6 years ago
Depends on: 664087
(Assignee)

Updated

6 years ago
Blocks: 638758
Depends on: 668941
Blocks: 695640
(Assignee)

Updated

6 years ago
Blocks: 557520
You need to log in before you can comment on or make changes to this bug.