[Webkit-unassigned] [Bug 23556] On RTL pages, horizontal scrollbar is missing

[bugzilla-daemon at webkit.org]

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


Hironori Bono <hbono at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #43559|0                           |1
        is obsolete|                            |
  Attachment #45958|                            |review?
               Flag|                            |




--- Comment #21 from Hironori Bono <hbono at chromium.org>  2010-01-06 03:31:22 PST ---
Created an attachment (id=45958)
 --> (https://bugs.webkit.org/attachment.cgi?id=45958)
Workaround fix 4

Jeremy and Maciej,

Thank you for your review and sorry for my slow response.

(In reply to comment #17)
> (From update of attachment 43559 [details])
> I'm not a reviewer, but here are my comments.
> Hyatt or Dan should probably give their opinion on whether there's a better
> fix.
> 
> > Index: WebCore/ChangeLog
> > ===================================================================
> > +        // Update the minimal left position.
> Can you add some more details and point to the bug URL from the comment.

Thank you for noticing this. I have added some more comments why we need the
leftmost position to fix this issue.

> > +        m_rtlMinChildPosition = min(chPos, m_rtlMinChildPosition);
> ASSERT(m_rtlMinChildPosition <= 0);

Done.

> > +    // Reset the minimal left position for RTL child elements before start layouting children.
> // Reset before preforming child layout.
> > +    m_rtlMinChildPosition = 0;
> 
> > +    // Override the RenderBox::marginLeft().
> // Override RenderBox::marginLeft().

Done.

> > Property changes on: LayoutTests/fast/text/international/rtl-unexpected-underflow.html
> > ___________________________________________________________________
> > Name: svn:executable
> >    + *
> Is this OK?

Oops, I forgot changing the file permission and line-endings when I copied this
file from Windows. I have changed them to remove this property.

(In reply to comment #20)
> (From update of attachment 43559 [details])
> Please try to pare down the comments. It's not necessary to have a comment
> (often lengthy) every time int m_rtlMinChildPosition appears. This hurts
> readability. Try to limit your comments to saying *why* the code is doing
> something, rather than *what* it is doing. "what" should be apparent from the
> code. I also agree with Jeremy's comment that the "less than or equal to 0"
> condition is best documented with an ASSERT, in which case I expect the
> multiple comments explaining that it is less than or equal to 0 are not
> necessary.
> 
> r- to clean this up. The change otherwise looks ok to me, assuming it matches
> what other browsers do, but it may help to have a rendering expert chime in.

Thank you for your comments. I tend to write more garbage comments when I'm
less confident of it. I deleted some garbage comments in my previous change.
Nevertheless, I'm still not so confident of this change because 'offsetLeft'
values are not so consistent among browsers and I'm not sure what values I
should rely on. (This change makes Safari look similar with other browsers,
though.)

Regards,

Hironori Bono

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