Bug 48479 - queryCommandValue should fall back to queryCommandState
Summary: queryCommandValue should fall back to queryCommandState
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-27 15:53 PDT by Ryosuke Niwa
Modified: 2011-09-16 11:41 PDT (History)
8 users (show)

See Also:


Attachments
fixes the bug (5.47 KB, patch)
2010-10-27 16:54 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-10-27 15:53:57 PDT
queryCommandValue should return 'true' or 'false' when the command doesn't return any string and queryCommandState is supported.  This is consistent with IE and will greatly improve WebKit's score on http://www.browserscope.org/richtext2/test
Comment 1 Ryosuke Niwa 2010-10-27 16:54:11 PDT
Created attachment 72113 [details]
fixes the bug
Comment 2 Darin Adler 2010-10-27 17:19:29 PDT
Comment on attachment 72113 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=72113&action=review

I’m surprised that this functions works that way, but change looks OK. Do any of the test cases cover the mapping of mixed state to "false"?

> LayoutTests/editing/execCommand/script-tests/query-text-alignment.js:7
> +function isEquivalentBoolean(string, bool) {

The function name here isn't all that clear.
Comment 3 Ryosuke Niwa 2010-10-27 17:59:36 PDT
(In reply to comment #2)
> I’m surprised that this functions works that way, but change looks OK. Do any of the test cases cover the mapping of mixed state to "false"?

I'm pretty certain that query-command-state.js do test that.

> > LayoutTests/editing/execCommand/script-tests/query-text-alignment.js:7
> > +function isEquivalentBoolean(string, bool) {
> 
> The function name here isn't all that clear.

I don't like that name either but couldn't come up with a better name.  isEquivalentBooleanValue?  isBooleanValueEqualTo?  isBooleanEqualTo?
Comment 4 Ryosuke Niwa 2010-10-28 14:20:50 PDT
(In reply to comment #3)
> > > LayoutTests/editing/execCommand/script-tests/query-text-alignment.js:7
> > > +function isEquivalentBoolean(string, bool) {
> > 
> > The function name here isn't all that clear.
> 
> I don't like that name either but couldn't come up with a better name.  isEquivalentBooleanValue?  isBooleanValueEqualTo?  isBooleanEqualTo?

Tony pointed out that we can just do centerValue != center.toString() and get rid of the function.  I'll land after making this change.
Comment 5 Ryosuke Niwa 2010-10-28 14:39:00 PDT
Committed r70810: <http://trac.webkit.org/changeset/70810>
Comment 6 Aryeh Gregor 2011-09-16 11:41:21 PDT
For anyone interested, this is currently being discussed:

http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-September/033235.html