Bug 45910 - queryCommandState with justifyCenter, justifyLeft, and justifyRight always return false
Summary: queryCommandState with justifyCenter, justifyLeft, and justifyRight always re...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
: 17593 (view as bug list)
Depends on: 46382
Blocks: 46954
  Show dependency treegraph
 
Reported: 2010-09-16 11:36 PDT by Ryosuke Niwa
Modified: 2010-09-30 19:16 PDT (History)
11 users (show)

See Also:


Attachments
work in progress (3.33 KB, patch)
2010-09-16 14:02 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
work in progress with a test (17.04 KB, patch)
2010-09-23 12:54 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (19.26 KB, patch)
2010-09-27 15:13 PDT, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2010-09-16 11:36:37 PDT
WebKit does not implement queryCommandValue for justifyCenter, justifyLeft, and justifyRight.
Comment 1 Ryosuke Niwa 2010-09-16 13:47:58 PDT
MSIE doesn't support text-align CSS property but only responds to align attribute

When "world" is selected, Firefox returns left for the following markups:
<div style="text-align: left;">hello<br><span style="text-align: right;">world</span></div>
<div style="text-align: left;">hello world<br><span style="display: block; text-align: right;">WebKit</span></div>

but I don't think we should be returning left for the latter case.  It should be right instead because text-align: is respected for all block-level elements.
Comment 2 Ryosuke Niwa 2010-09-16 14:02:21 PDT
Created attachment 67841 [details]
work in progress

This fails in the case of <div>hello<span style="text-align:right;">world</span></div> because we return right in such case but text is clearly left-aligned.
Comment 3 Ryosuke Niwa 2010-09-16 15:40:20 PDT
*** Bug 17593 has been marked as a duplicate of this bug. ***
Comment 4 Ryosuke Niwa 2010-09-16 16:09:42 PDT
I'm getting -webkit-left and left, and I'm not sure what's the difference between them.  I was speculating that there's some difference in how they behave in the context of RTL but it doesn't seem to be the case as far as I've tested so far.

Dan, do you know if I can treat them equally for the purpose of queryCommandState?
Comment 5 Ryosuke Niwa 2010-09-23 12:54:46 PDT
Created attachment 68572 [details]
work in progress with a test

I did look into the difference between -webkit-left and left and it seems like the only notable difference is in RenderBox::calcHorizontalMargins where we have:

    } else if ((marginRight.isAuto() && width() < containerWidth)
            || (!marginLeft.isAuto() && containingBlock()->style()->direction() == RTL && containingBlock()->style()->textAlign() == WEBKIT_LEFT)) {
        m_marginLeft = marginLeft.calcValue(containerWidth);
        m_marginRight = containerWidth - width() - m_marginLeft;
    } else if ((marginLeft.isAuto() && width() < containerWidth)
            || (!marginRight.isAuto() && containingBlock()->style()->direction() == LTR && containingBlock()->style()->textAlign() == WEBKIT_RIGHT)) {
        m_marginRight = marginRight.calcValue(containerWidth);
        m_marginLeft = containerWidth - width() - m_marginRight;

But most importantly, RenderText::localCaretRect, RenderBlock::computeHorizontalPositionsForLine,  and RenderBlock::localCaretRect treat WEBKIT_LEFT and LEFT equally (likewise for WEBKIT_RIGHT, WEBKIT_CENTER) so I'm quite convinced that we can treat -webkit-(left|right|center) and (left|right|center) equally in editing.  In this new work-in-progress patch, I treat them equally in getPropertiesNotIn.  Some tests fail due to the bug 41989 and the bug 46382.
Comment 6 Ryosuke Niwa 2010-09-27 15:13:06 PDT
Created attachment 68971 [details]
Patch
Comment 7 Ryosuke Niwa 2010-09-27 18:23:11 PDT
Committed r68465: <http://trac.webkit.org/changeset/68465>