Bug 41989 - stateStyle (@EditorCommand.cpp) should ask EditingBehavior for platform specific behavior
Summary: stateStyle (@EditorCommand.cpp) should ask EditingBehavior for platform speci...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on: 39854
Blocks: 46382
  Show dependency treegraph
 
Reported: 2010-07-09 14:30 PDT by Antonio Gomes
Modified: 2010-09-27 14:11 PDT (History)
6 users (show)

See Also:


Attachments
fixed both 41989 and 46382 (26.71 KB, patch)
2010-09-24 23:43 PDT, Ryosuke Niwa
tonikitoo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2010-07-09 14:30:33 PDT
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.
-------------------------------
Comment 1 Ryosuke Niwa 2010-09-16 15:45:12 PDT
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.
Comment 2 Antonio Gomes 2010-09-16 16:06:23 PDT
(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.
Comment 3 Ryosuke Niwa 2010-09-16 16:43:45 PDT
(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.
Comment 4 Antonio Gomes 2010-09-16 16:48:56 PDT
(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:)
Comment 5 Ryosuke Niwa 2010-09-16 17:09:23 PDT
(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.
Comment 6 Ryosuke Niwa 2010-09-23 12:09:44 PDT
(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.
Comment 7 Antonio Gomes 2010-09-23 12:12:24 PDT
(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.
Comment 8 Ryosuke Niwa 2010-09-24 23:43:57 PDT
Created attachment 68812 [details]
fixed both 41989 and 46382
Comment 9 Ryosuke Niwa 2010-09-24 23:45:52 PDT
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.
Comment 10 Ryosuke Niwa 2010-09-24 23:49:48 PDT
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?
Comment 11 Ryosuke Niwa 2010-09-25 20:06:54 PDT
Hi Antonio, Should I delete the failing tests for now?  Or should I keep them?
Comment 12 Antonio Gomes 2010-09-25 20:13:40 PDT
(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?
Comment 13 Ryosuke Niwa 2010-09-25 20:20:48 PDT
(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.
Comment 14 Antonio Gomes 2010-09-25 20:40:09 PDT
Comment on attachment 68812 [details]
fixed both 41989 and 46382

Hum ... thinking. Resetting r? so we decide before landing.
Comment 15 Ryosuke Niwa 2010-09-27 10:53:02 PDT
(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?
Comment 16 Antonio Gomes 2010-09-27 11:07:01 PDT
(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.
Comment 17 Ryosuke Niwa 2010-09-27 11:16:22 PDT
(In reply to comment #16)
> consider my r+, then.

You menat r=you, correct?  (just making sure).
Comment 18 Antonio Gomes 2010-09-27 11:17:09 PDT
Comment on attachment 68812 [details]
fixed both 41989 and 46382

r=me, yeap
Comment 19 Ryosuke Niwa 2010-09-27 14:11:09 PDT
Committed r68423: <http://trac.webkit.org/changeset/68423>