[Webkit-unassigned] [Bug 52128] ISO-8859-8 Hebrew text displayed reversed with dir="rtl"

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=52128


Xiaomei Ji <xji at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #78755|0                           |1
        is obsolete|                            |




--- Comment #12 from Xiaomei Ji <xji at chromium.org>  2011-02-01 12:05:07 PST ---
Created an attachment (id=80800)
 --> (https://bugs.webkit.org/attachment.cgi?id=80800&action=review)
patch w/ layout test

(In reply to comment #11)
> (From update of attachment 78755 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78755&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        Fix rendering of ISO-8859-8 when dir="rtl".
> > +        https://bugs.webkit.org/show_bug.cgi?id=52128
> > +
> > +        Test: fast/dom/iso-8859-8.html
> > +
> > +        * platform/text/BidiResolver.h:
> > +        (WebCore::::createBidiRunsForLine):
> > +        * rendering/RenderBlockLineLayout.cpp:
> > +        (WebCore::RenderBlock::constructLine):
> > +        (WebCore::RenderBlock::layoutInlineChildren):
> > +
> 
> This should explain what is changing and why. I can’t even review the patch without knowing what it’s trying to do.

Done.


> 
> > Source/WebCore/platform/text/BidiResolver.h:965
> > +    } else if (direction == RTL) {
> > +        reverseRuns(0, runCount() - 1);
> >      }
> 
> There shouldn’t be braces around a single-line block.

Done.
Thought it needs to be braces due to the braces in "if" part.

> 
> > LayoutTests/ChangeLog:9
> > +        * fast/dom/iso-8859-8.html: Added.
> 
> This is not a DOM change. Please don’t put the test in fast/dom.

Moved to fast/text/international/

> 
> > LayoutTests/fast/dom/iso-8859-8.html:55
> > +    function checkRenderingResult(sel, node, offset, direction, element, index) {
> > +        for (var i = 0; i < element.innerText.length; ++i) {
> > +            sel.setBaseAndExtent(node, offset, node, offset);
> > +            if (direction == "ltr") {
> > +                sel.modify("move", "right", "character");
> > +                sel.modify("extend", "left", "character");
> > +            } else {
> > +                sel.modify("move", "left", "character");
> > +                sel.modify("extend", "right", "character");
> > +            }
> > +            if (direction == "ltr") {
> > +                assertEqual("Test " + index + " " + direction + " the " + i + "th character from left", sel.toString(), element.innerText[i]);
> > +            } else {
> > +                assertEqual("Test " + index + " " + direction + " the " + i + "th character from right", sel.toString(), element.innerText[i]);
> > +            }
> > +            node = sel.baseNode;
> > +            offset = sel.baseOffset;
> > +        }
> > +    }
> 
> Using the non-trivial logic for modifying selection is a very indirect way to test this change. The best test for this would be a render tree dump. Those include the directionality (and override state) of individual text runs, which should suffice. If you can’t do that, then you can test directly for the visual position of characters using Range.getClientRects().

You're right. Changed to use dumpRenderTree. I was thinking to make the expected result cross-platform.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.