1069941 - Wrong padding on Arabic-indic digits on bidi documents, probably a regression

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[ebrahim]

Attachment: Firefox33Chrome37.png

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140911151253

Steps to reproduce:

1. Open data:text/html;charset=utf8,<div dir="rtl"><span dir="ltr" style="padding-left: 13px;">+٥</span>


Actual results:

Specific padding is placed inside the element, between the digit (٥) and plus.


Expected results:

Compare it with Chrome, the padding shouldn't be there.

The testcase is simplified from https://ckb.wikipedia.org/wiki/%D8%AF%D8%A7%DA%95%DB%8E%DA%98%DB%95:Significant_figures/rnd which is really annoying. It is probably a regression happened from six month ago.

Comment 1

[Jonathan Kew [:jfkthame]]

Interestingly, the example

data:text/html;charset=utf8,<div dir="rtl"><span dir="ltr" style="padding-right: 13px;">+٥</span>

also looks wrong, with the padding once again appearing between the digit and the plus.

Adding a border to the span, as in

data:text/html;charset=utf8,<div dir="rtl"><span dir="ltr"
   style="padding-left: 13px; border: 1px solid red;">+٥</span>

data:text/html;charset=utf8,<div dir="rtl"><span dir="ltr"
   style="padding-right: 13px; border: 1px solid red;">+٥</span>

shows that the two are not in fact identical, though they are both wrong.

Almost certainly a regression from some part of the physical-to-logical coordinate conversions.

Comment 2

[Jonathan Kew [:jfkthame]]

Last good nightly: 2014-03-12
First bad nightly: 2014-03-13
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=44ae8462d6ab&tochange=46041cc216fd

Most likely candidate there (by a long way) sounds like changeset dab8e3865967, which was "patch 2" in bug 789096.

Comment 3

[Jonathan Kew [:jfkthame]]

May turn out to be a dupe of bug 1067268?

Comment 4

[Jonathan Kew [:jfkthame]]

And here's a related example with the directions swapped; in this case, the + and digit end up overlapping:

data:text/html;charset=utf8,<div dir="ltr"><span dir="rtl" style="padding-left:10px;">+٥</span>

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Reftest showing regression in handling of borders (or padding) in mixed-dir inline cases.

This bug can also be demonstrated using borders. Here's a reftest that fails on current trunk, but would have passed prior to the regression. The text is transparent to avoid any dependence on exact glyph rasterization; the misrendering of the borders is enough.

Comment 6

[Jonathan Kew [:jfkthame]]

Here's an example that shows the same problem happening with margins:

data:text/html,
  <span style="display:inline-block;border:1px solid red">
    <span dir="rtl" style="margin-left: 50px;">(12)</span></span>

The underlying problem is the same in all these cases, I believe. Note that the parentheses here, or the plus sign in the earlier examples, will resolve to RTL directionality, while the digits are LTR. Similar testcases can be constructed using a mix of Arabic or Hebrew and Latin letters, for example.

Comment 7

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #3)
> May turn out to be a dupe of bug 1067268?

How so when the regression range is completely different?

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #7)
> (In reply to Jonathan Kew (:jfkthame) from comment #3)
> > May turn out to be a dupe of bug 1067268?
> 
> How so when the regression range is completely different?

Sorry, forget I said that.... I hadn't compared the regression ranges at that point. I had glanced (too briefly!) at the testcase and thought it looked related.

Comment 9

[Simon Montagu :smontagu]

Attachment: Tests

I modified your test slightly not to use red for a non-failure condition, moved it to layout/reftest/bidi instead of bugs, and added a test for the margin case.

Comment 10

[Simon Montagu :smontagu]

Attachment: Patch 1: determine the first and last inlines on the line more accurately

This fixes the border and padding testcases, but not the margin one.

Comment 11

[Simon Montagu :smontagu]

Attachment: Patch 2: reset the inline-start before ReorderFrames instead of inside RepositionInlineFrames

This fixes the margin testcase -- originally we were making a stab in the dark to guess which frame had the margin before reordering.

Comment 12

[Simon Montagu :smontagu]

Attachment: Patch 3: cosmetic fixes

While I'm here, this changes some variable names and moves some stuff around in the hope that the code will become clearer.

Comment 13

[Simon Montagu :smontagu]

Attachment: Patch 1 v.2: determine the first and last inlines on the line more accurately

Oops, I left part of the comments out-of-date.

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 8497454 [details] [diff] [review]
Patch 1 v.2: determine the first and last inlines on the line more accurately

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

The code looks sensible, but I think one of the (important) comments needs fixing. r+ with that.

::: layout/base/nsBidiPresUtils.cpp
@@ +1415,5 @@
> +  // writing mode, but we're traversing the frames in the order of the
> +  // container's writing mode. To get the right values, we set start and
> +  // end margins on a logical margin in the frame's writing mode, and
> +  // then convert the margin to the container's writing mode to set the
> +  // coordinates

nit: add period.

::: layout/base/nsBidiPresUtils.h
@@ +423,4 @@
>     *
> +   * N.B: "First appearance" and "Last appearance" in the previous
> +   * paragraph refer to the frame's inline direction, not necessarily
> +   * the line's

nit: Add period, please.

@@ +428,5 @@
> +   * @param aContinuationStates  A map from nsIFrame* to
> +   *                              nsFrameContinuationState
> +   * @param[in] aSpanInLineOrder TRUE means that the inline direction
> +   *                              of the frame is different from that of
> +   *                              its container

Isn't this comment backwards? AFAICT, true means that the inline direction of the span matches that of its container.

I think I'd prefer a name more like aSpanDirMatchesLineDir; WDYT?

Comment 15

[Jonathan Kew [:jfkthame]]

Comment on attachment 8497445 [details] [diff] [review]
Patch 2: reset the inline-start before ReorderFrames instead of inside RepositionInlineFrames

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

::: layout/generic/nsLineLayout.cpp
@@ +2597,5 @@
>          break;
>      }
>    }
>  
> +  aLine->IndentBy(dx, mContainerWidth);

Might be worth putting this within an |if (dx) { ... }| condition, given that dx will very often be zero?

Or do it within each of the |if| blocks below, so that in the (most common) case where neither of them is taken, we skip this as well.

Comment 16

[ebrahim]

Thank you for fixing the bug. It would be nice if you guys check if the patch is fixing the original issue https://ckb.wikipedia.org/wiki/%D8%AF%D8%A7%DA%95%DB%8E%DA%98%DB%95:Significant_figures/rnd also

Comment 17

[Jonathan Kew [:jfkthame]]

Comment on attachment 8497446 [details] [diff] [review]
Patch 3: cosmetic fixes

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

Looks OK, except that I think the commit message is not entirely accurate :) ... see below.

::: layout/base/nsBidiPresUtils.cpp
@@ +1463,5 @@
>                        aIsEvenLevel,
>                        iCoord,
>                        aContinuationStates,
>                        frameWM,
> +                      aFrame->GetLogicalSize(aContainerWM).Width(aContainerWM));

I think you're actually fixing a bug, as we used to pass the inline-size but we really do want a physical width here. The bug wouldn't show up until vertical mode is enabled, but still...this isn't purely a cosmetic change, is it?

Comment 18

[Jonathan Kew [:jfkthame]]

(In reply to ebrahim from comment #16)
> Thank you for fixing the bug. It would be nice if you guys check if the
> patch is fixing the original issue
> https://ckb.wikipedia.org/wiki/%D8%AF%D8%A7%DA%95%DB%8E%DA%98%DB%95:
> Significant_figures/rnd also

I've checked that page in a patched build, and it looks fixed to me. Once the patches are checked-in, please test a Nightly build yourself as well and let us know if you find any remaining issues - thanks!

Comment 19

[Simon Montagu :smontagu]

> > +   * @param[in] aSpanInLineOrder TRUE means that the inline direction
> > +   *                              of the frame is different from that of
> > +   *                              its container
> 
> Isn't this comment backwards? AFAICT, true means that the inline direction
> of the span matches that of its container.

Yes, you're quite right (the comment was originally correct when the flag was aReversedSpan, but I changed the flag and didn't change the comment)
 
> I think I'd prefer a name more like aSpanDirMatchesLineDir; WDYT?

Changed.

Comment 20

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a094aba8db2
https://hg.mozilla.org/integration/mozilla-inbound/rev/64a61b49574b
https://hg.mozilla.org/integration/mozilla-inbound/rev/d591907bdb78
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0eda891a74d

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1a094aba8db2
https://hg.mozilla.org/mozilla-central/rev/64a61b49574b
https://hg.mozilla.org/mozilla-central/rev/d591907bdb78
https://hg.mozilla.org/mozilla-central/rev/c0eda891a74d