Bug 41989 – stateStyle (@EditorCommand.cpp) should ask EditingBehavior for platform specific behavior

• Component: DOM
• Status: RESOLVED FIXED

[Antonio Gomes]

Spin off of the discussion in bug 39854, specially in https://bugs.webkit.org/show_bug.cgi?id=39854#c44 and https://bugs.webkit.org/show_bug.cgi?id=39854#c52

This won't be a refactor only (tests needed), and needs to work for the following situations (detailed by Ojan):

------------------------
When shouldConsiderStylePresentOnlyIfThroughoutTheSelection is true it's taking what used to be a tri-state and making it a boolean. Specifically, I think we'd return the wrong value in the following case:
<div id=foo contentEditable>foo<b>bar</b></div>
<script>
window.getSelection().selectAllChildren(foo);
console.log(document.queryCommandIndeterm('bold'));
</script>

That should log "true" to the console. With your patch, I think it would log "false". On the other hand, this patch fixes queryCommandState to return the correct value in this case. Currently it would return true for the above and should return false.

It would be unfortunate to fix one case and break the other. Can we do this as two separate patches?

1. Just change the current editingBehavior call. Call the method shouldToggleStyleBasedOnStartOfSelection, but also put a FIXME to use that method in stateStyle for the cases where it's used for queryCommandState.
2. Rename shouldToggleStyleBasedOnStartOfSelection to shouldConsiderStylePresentOnlyIfThroughoutTheSelection and use it in stateStyle, but not if it's being used to get a return value for queryCommandIndeterm. This is a bit complicated because right now both queryCommandState and queryCommandIndeterm end up calling the same method. So it would require a few more changes to EditorCommand.cpp.
-------------------------------

[Ryosuke Niwa]

Yes, this is a pending issue we need to work on.  I'm planning on getting to it once the bug 27818 is resolved since that needs to be fixed before we can implement queryCommandState for underline & strikeThrough correctly.

[Antonio Gomes]

(In reply to comment #1)
> Yes, this is a pending issue we need to work on.  I'm planning on getting to it once the bug 27818 is resolved since that needs to be fixed before we can implement queryCommandState for underline & strikeThrough correctly.

Niwa, I have a patch for that. Uploading soon, unless your work on it is already done.

[Ryosuke Niwa]

(In reply to comment #2)
> (In reply to comment #1)
> > Yes, this is a pending issue we need to work on.  I'm planning on getting to it once the bug 27818 is resolved since that needs to be fixed before we can implement queryCommandState for underline & strikeThrough correctly.
> 
> Niwa, I have a patch for that. Uploading soon, unless your work on it is already done.

You have a patch for 27818?  That's a great news for me.  I was intending to work on that in the next couple of weeks but I can definitely take a look at your patch if you already have a working patch.

[Antonio Gomes]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > Yes, this is a pending issue we need to work on.  I'm planning on getting to it once the bug 27818 is resolved since that needs to be fixed before we can implement queryCommandState for underline & strikeThrough correctly.
> > 
> > Niwa, I have a patch for that. Uploading soon, unless your work on it is already done.
> 
> You have a patch for 27818?  That's a great news for me.  I was intending to work on that in the next couple of weeks but I can definitely take a look at your patch if you already have a working patch.

no. to this one:)

[Ryosuke Niwa]

(In reply to comment #4)
> > You have a patch for 27818?  That's a great news for me.  I was intending to work on that in the next couple of weeks but I can definitely take a look at your patch if you already have a working patch.
> 
> no. to this one:)

Ah, ok.  It's good to know as well.  I wanted to this bug last summer but got carried away by 27818.

By the way, I forgot to mention that
(In reply to comment #2)
> Niwa, I have a patch for that. Uploading soon, unless your work on it is already done.

My first name is Ryosuke although many people mistake Niwa as my first name since it's shorter and more pronounceable.  But you can call me Niwa too.  It's super confusing too that many (if not all) contributors from Tokyo put their last name first in capitals.

[Ryosuke Niwa]

(In reply to comment #4)
> no. to this one:)

Hi Antonio, will you be able to post the patch you have?  If it's too much trouble for you, then I'll make a patch myself since this bug seems to be an easy fix.

[Antonio Gomes]

(In reply to comment #6)
> (In reply to comment #4)
> > no. to this one:)
> 
> Hi Antonio, will you be able to post the patch you have?  If it's too much trouble for you, then I'll make a patch myself since this bug seems to be an easy fix.

Ah it's been always bypassing my attention =/.

I just have the patch at pc at home. You give me until tonight to fix that? :) if I fail, please do. thank.

[Ryosuke Niwa]

Created attachment 68812 [details]
fixed both 41989 and 46382

[Ryosuke Niwa]

The actual change is very small but I added lots of tests in this patch because we don't currently have any adequate test coverage for queryCommandState.  The fact this patch didn't require any rebaseline at all is a really bad sign.

[Ryosuke Niwa]

There are currently 4 tests that always fail in my patch:

FAIL queryCommandState("subscript") returns false when selecting all of "<sub><div>hello world</div></sub>", expected true
FAIL queryCommandState("subscript") returns false when selecting second word of "<sup><sub><div>hello world WebKit</div></sub></sup>", expected true

FAIL queryCommandState("superscript") returns false when selecting all of "<sup><div>hello world</div></sup>", expected true
FAIL queryCommandState("superscript") returns false when selecting second word of "<sup><sub><div>hello world WebKit</div></sub></sup>", expected true

WebKit renders as if hello world is sub/sup in these four cases but computed style says the vertical-align is baseline.  Does anyone know if this is the desired behavior for CSS2.1 or the latest draft of CSS3?

[Ryosuke Niwa]

Hi Antonio, Should I delete the failing tests for now?  Or should I keep them?

[Antonio Gomes]

(In reply to comment #11)
> Hi Antonio, Should I delete the failing tests for now?  Or should I keep them?

This bug ideally should introduce no behavior change. Are they now failing because of this:

 bool Document::queryCommandState(const String& commandName)
 {
-    return command(this, commandName).state() != FalseTriState;
+    return command(this, commandName).state() == TrueTriState;
 }


If so we could change it in a follow up?

[Ryosuke Niwa]

(In reply to comment #12)
> (In reply to comment #11)
> This bug ideally should introduce no behavior change. Are they now failing because of this:

No, they are failing because of our vertical-align implementation.  See the comment #10.  So when we have <sub><div>hello</div></sub>, computed style on "hello" indicates that the vertical alignment is baseline, which isn't really want I expect because the text appears to be having sub vertical alignment.  And I'm not sure if this behavior is desired or it's a bug.

[Antonio Gomes]

Comment on attachment 68812 [details]
fixed both 41989 and 46382

Hum ... thinking. Resetting r? so we decide before landing.

[Ryosuke Niwa]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > This bug ideally should introduce no behavior change. Are they now failing because of this:
> 
> No, they are failing because of our vertical-align implementation.  See the comment #10.  So when we have <sub><div>hello</div></sub>, computed style on "hello" indicates that the vertical alignment is baseline, which isn't really want I expect because the text appears to be having sub vertical alignment.  And I'm not sure if this behavior is desired or it's a bug.

I did some investigation on this issue and turned out that in <sub><div>hello</div></sub>, hello inherits the small font size but not the sub vertical alignment.  This is clear from the rendering of:
<span style="vertical-align:sub;"><div>WebKit<span style="vertical-align:sub;">hello</span></div></span>world

So I should just delete my test cases.  Do you want me to submit the patch again for a review or should I just delete them and commit?

[Antonio Gomes]

(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (In reply to comment #11)
> > > This bug ideally should introduce no behavior change. Are they now failing because of this:
> > 
> > No, they are failing because of our vertical-align implementation.  See the comment #10.  So when we have <sub><div>hello</div></sub>, computed style on "hello" indicates that the vertical alignment is baseline, which isn't really want I expect because the text appears to be having sub vertical alignment.  And I'm not sure if this behavior is desired or it's a bug.
> 
> I did some investigation on this issue and turned out that in <sub><div>hello</div></sub>, hello inherits the small font size but not the sub vertical alignment.  This is clear from the rendering of:
> <span style="vertical-align:sub;"><div>WebKit<span style="vertical-align:sub;">hello</span></div></span>world
> 
> So I should just delete my test cases.  Do you want me to submit the patch again for a review or should I just delete them and commit?

consider my r+, then.

[Ryosuke Niwa]

(In reply to comment #16)
> consider my r+, then.

You menat r=you, correct?  (just making sure).

[Antonio Gomes]

Comment on attachment 68812 [details]
fixed both 41989 and 46382

r=me, yeap

[Ryosuke Niwa]

Committed r68423: <http://trac.webkit.org/changeset/68423>