Summary: | stateStyle (@EditorCommand.cpp) should ask EditingBehavior for platform specific behavior | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antonio Gomes <tonikitoo> | ||||
Component: | DOM | Assignee: | Ryosuke Niwa <rniwa> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | darin, mrobinson, ojan, rniwa, tkent, tony | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | All | ||||||
Bug Depends on: | 39854 | ||||||
Bug Blocks: | 46382 | ||||||
Attachments: |
|
Description
Antonio Gomes
2010-07-09 14:30:33 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. (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. (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. (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:) (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. (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. (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. Created attachment 68812 [details]
fixed both 41989 and 46382
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. 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? Hi Antonio, Should I delete the failing tests for now? Or should I keep them? (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? (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 on attachment 68812 [details]
fixed both 41989 and 46382
Hum ... thinking. Resetting r? so we decide before landing.
(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? (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. (In reply to comment #16) > consider my r+, then. You menat r=you, correct? (just making sure). Comment on attachment 68812 [details]
fixed both 41989 and 46382
r=me, yeap
Committed r68423: <http://trac.webkit.org/changeset/68423> |