Bug 21033 - queryCommandValue('FontSize') returns pixel values instead of IE font numbers
Summary: queryCommandValue('FontSize') returns pixel values instead of IE font numbers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 525.x (Safari 3.1)
Hardware: All All
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: GoogleBug, InRadar, NeedsReduction, ReviewedForRadar
Depends on:
Blocks:
 
Reported: 2008-09-23 13:01 PDT by linda167
Modified: 2010-09-09 14:29 PDT (History)
13 users (show)

See Also:


Attachments
test case (2.45 KB, text/html)
2009-01-22 14:32 PST, Eric Seidel
no flags Details
nicer test case (2.60 KB, text/html)
2009-01-22 14:44 PST, Eric Seidel
no flags Details
updated test case, including results affected by bug 19161 (3.50 KB, text/html)
2009-07-28 13:08 PDT, Eric Seidel
no flags Details
work in progress (5.80 KB, patch)
2010-09-08 15:17 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (17.06 KB, patch)
2010-09-08 16:46 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixed per Darin's comments (19.21 KB, patch)
2010-09-08 23:04 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 linda167 2008-09-23 13:01:26 PDT
Safari's execCommand('FontSize') takes in the standard browser font sizes (values range from 1-7). However, when calling queryCommandValue for FontSize, Safari gives us back bogus pixel values instead of 1-7. 

These pixel values are also not correct. If you measure the actual pixel of the text, they are much smaller than the pixel values Safari gives us.

The fix should be to return us the correct browser font size values (1-7). IE and Firefox currently does this.

Here's the table for the bogus pixel values:
Browser font size 1 -> Safari: 10 px (Actual font size: 8 px)
Browser font size 2 -> Safari: 13 px (Actual font size: 10 px)
Browser font size 3 -> Safari: 16 px (Actual font size: 12 px)
Browser font size 4 -> Safari: 18 px (Actual font size: 14 px)
Browser font size 5 -> Safari: 24 px (Actual font size: 18 px)
Browser font size 6 -> Safari: 32 px (Actual font size: 24 px)
Browser font size 7 -> Safari: 48 px (Actual font size: 36 px)
Comment 1 Mark Rowe (bdash) 2008-09-23 14:12:37 PDT
<rdar://problem/6240811>
Comment 2 Eric Seidel 2009-01-22 14:32:54 PST
Created attachment 26941 [details]
test case

This will be a simple fix.  Here's a fancy test case I wrote to see what this is actually "supposed" to do.
Comment 3 Eric Seidel 2009-01-22 14:44:22 PST
Created attachment 26942 [details]
nicer test case
Comment 4 Eric Seidel 2009-02-13 11:28:19 PST
I've not seen the pixel values being "bogus".   But they aren't the IE font size numbers which FF and IE return.  I guess we should match them there.
Comment 5 Jon@Chromium 2009-03-30 14:37:49 PDT
This is causing Chromium Bug 7447.
Comment 6 Jon@Chromium 2009-03-30 14:38:26 PDT
*** Bug 24855 has been marked as a duplicate of this bug. ***
Comment 7 Eric Seidel 2009-03-30 15:18:36 PDT
The "FontSize" numbers which IE/FF return are pretty useless.  But that's how this command is "supposed" to work.  Now we just need to find out what pixel values map to what font size values in IE/FF.
Comment 8 Eric Seidel 2009-03-30 15:31:13 PDT
I think the code to do this kind of conversion is found in:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleUtil.cpp
But I'm not really sure.  Either way, it's a crazy amount of code for this support.

Gmail certainly can work around this behavior in WebKit by doing their own mapping from px values to IE font sizes (Which TrogEdit already does).

We still should fix this.  I'm just not sure what to fix it to yet.
Comment 9 Eric Seidel 2009-03-30 16:24:30 PDT
FontDescription optionally stores an m_keywordSize, which is exactly the size we'd want here.

But we'd have to plumb trough a method like:
String Frame::selectionStartStylePropertyValue(int stylePropertyID) const

which returned a FontDescription or an int keywordSize.

To calculate the keywordSize from the FontDescription, we'd have to test to see if it's cached first, if it's not cached on the FontDescription, then we'd have to approximate it based on the font size.  The logic to approximate it may depend on quirks vs. standards mode, and may be rather complicated, I don't know.
Comment 10 Eric Seidel 2009-03-30 17:15:40 PDT
Looks like we have some similar mapping code to mozilla in:
float CSSStyleSelector::fontSizeForKeyword(int keyword, bool quirksMode, bool fixed) const

Probably because Hyatt worked on both implementations. :)
Comment 11 Jon@Chromium 2009-07-08 15:42:18 PDT
Eric, I would really like to have this fixed for the 3.0 release of Chrome.
Comment 12 Jon@Chromium 2009-07-20 10:35:09 PDT
Ojan or Julie could you pick this up?  This bug is really embarrassing.  I want it fixed for 3.0 and we don't appear to be making any progress.
Comment 13 Julie Parent 2009-07-20 10:51:20 PDT
I don't think this is actually a dupe of https://bugs.webkit.org/show_bug.cgi?id=24855.  Jon, which of these 2 are higher priority?  I believe this one here is worked around properly in all Google products, so is the other one (Gmail users can't change font size) the higher priority?
Comment 14 Jon@Chromium 2009-07-20 16:31:31 PDT
Yes, this is a lower priority that the one for which there is no workaround.
Comment 15 Eric Seidel 2009-07-28 12:52:13 PDT
My test case broke after changes to execCommand to have it remove uneeded spans.  Will fix.
Comment 16 Eric Seidel 2009-07-28 13:08:31 PDT
Created attachment 33663 [details]
updated test case, including results affected by bug 19161
Comment 17 Ryosuke Niwa 2010-09-08 15:17:58 PDT
Created attachment 66948 [details]
work in progress
Comment 18 Darin Adler 2010-09-08 15:47:06 PDT
Generally looks like a good approach.

Typo: "lagacy".
Comment 19 Ryosuke Niwa 2010-09-08 16:46:41 PDT
Created attachment 66960 [details]
Patch
Comment 20 Darin Adler 2010-09-08 17:28:36 PDT
Comment on attachment 66960 [details]
Patch

> +    Node* node = m_node.get();
> +    if (!node)
> +        return false;

This can just be:

    if (!m_node)

No need for get() or a local variable.

> +    RefPtr<RenderStyle> style = m_node->computedStyle(m_pseudoElementSpecifier);
> +    if (!style)
> +        return false;
> +
> +    return style->fontDescription().useFixedDefaultSize();

I don't think it makes sense to name this "isFontFixedSize" because it does not answer that question. Instead it should use a name consistent with the name in FontDescription such as "shouldUseFixedDefaultSize".

> +template<typename T>
> +static int findNearestLegacyFontSize(int size, const T* table, int multiplier)
> +{
> +    // Ignore table[0] because xx-small does not correspond to any legacy font size.
> +    for (int i = 1; i < totalKeywords - 1; i++) {
> +        if (size * 2 < (table[i] + table[i + 1]) * multiplier)
> +            return i;
> +    }
> +    return totalKeywords - 1;
> +}

Is this algorithm the copy of something done elsewhere? Why don't I see "-" lines in the place where the old code was? Are we making a new copy of the code without making the other copies share the code?

> +        // Given a font size in pixel, this return will return legacy font size between 1-7.

The phrase "between 1-7" is not good wording. You should say "between 1 and 7" or just "1-7".

> +        static int legacyFontSize(Document* document, int size, bool fixed);

The argument name "document" should be removed.

The argument name "size" is unclear. What kind of size? The function returns a size, so the argument has to be distinguished. Maybe pixelSize would be the right name.

The argument name "fixed" is not right. It should be "shouldUseFixedDefaultSize" I think.

> +    // If the pos is at the end of a text node, then this node is not fully selected.
> +    // Move it to the next deep quivalent position to avoid removing the style from this node.

Typo: "quivalent".

> +    // e.g. if pos was at Position("hello", 5) in <b>hello<div>world</div></b>, we want Position("world", 0) instead.
> +    // We only do this for range because caret at Position("hello", 5) in <b>hello</b>world should give you font-weight: bold.
> +    Node* posNode = pos.containerNode();
> +    if (selection()->isRange() && posNode && posNode->isTextNode() && pos.computeOffsetInContainerNode() == posNode->maxCharacterOffset())
> +        pos = nextVisuallyDistinctCandidate(pos);

Does the new test cover this case?

I worry that this special case is too specific.

I’ll say review- because I’m sure you’ll want to make at least one of the changes mentioned above.
Comment 21 Ryosuke Niwa 2010-09-08 22:42:23 PDT
(In reply to comment #20)
> (From update of attachment 66960 [details])
> > +    Node* node = m_node.get();
> > +    if (!node)
> > +        return false;
> 
> This can just be:
> 
>     if (!m_node)
> 
> No need for get() or a local variable.

Right, will fix.

> > +    RefPtr<RenderStyle> style = m_node->computedStyle(m_pseudoElementSpecifier);
> > +    if (!style)
> > +        return false;
> > +
> > +    return style->fontDescription().useFixedDefaultSize();
> 
> I don't think it makes sense to name this "isFontFixedSize" because it does not answer that question. Instead it should use a name consistent with the name in FontDescription such as "shouldUseFixedDefaultSize".

Sure.  Will rename it to useFixedFontDefaultSize.

> > +template<typename T>
> > +static int findNearestLegacyFontSize(int size, const T* table, int multiplier)
> > +{
> > +    // Ignore table[0] because xx-small does not correspond to any legacy font size.
> > +    for (int i = 1; i < totalKeywords - 1; i++) {
> > +        if (size * 2 < (table[i] + table[i + 1]) * multiplier)
> > +            return i;
> > +    }
> > +    return totalKeywords - 1;
> > +}
> 
> Is this algorithm the copy of something done elsewhere? Why don't I see "-" lines in the place where the old code was? Are we making a new copy of the code without making the other copies share the code?

No, I don't think this algorithm has been implemented elsewhere.  I'm more than happy to share the code if you can show me other place(s) where the same algorithm is used.

> > +        // Given a font size in pixel, this return will return legacy font size between 1-7.
> 
> The phrase "between 1-7" is not good wording. You should say "between 1 and 7" or just "1-7".

Right.  Also, "this return will return" should read "this function will return."  Will fix.

> > +        static int legacyFontSize(Document* document, int size, bool fixed);
> 
> The argument name "document" should be removed.
> 
> The argument name "size" is unclear. What kind of size? The function returns a size, so the argument has to be distinguished. Maybe pixelSize would be the right name.

Good point.  Will rename to pixelFontSize.

> The argument name "fixed" is not right. It should be "shouldUseFixedDefaultSize" I think.

Will fix.

> > +    // If the pos is at the end of a text node, then this node is not fully selected.
> > +    // Move it to the next deep quivalent position to avoid removing the style from this node.
> 
> Typo: "quivalent".

Will fix.


> > +    // e.g. if pos was at Position("hello", 5) in <b>hello<div>world</div></b>, we want Position("world", 0) instead.
> > +    // We only do this for range because caret at Position("hello", 5) in <b>hello</b>world should give you font-weight: bold.
> > +    Node* posNode = pos.containerNode();
> > +    if (selection()->isRange() && posNode && posNode->isTextNode() && pos.computeOffsetInContainerNode() == posNode->maxCharacterOffset())
> > +        pos = nextVisuallyDistinctCandidate(pos);
> 
> Does the new test cover this case?

Yes, without this change, we'll get wrong font size for monospace case.

> I worry that this special case is too specific.

The problem is that when we have <b>hello</b>world, and caret is at the boundary of hello and world, we want the queryCommandState('bold') to return true because when a user starts typing text, the text is bolded in this case.  However, if user had selected "world" and started typing text, the text that replaces "world" is not bolded.  As far as I'm concerned, the only way to distinguish this difference is to check whether the current selection is caret or not.  But I'm more than happy to take a different approach you know a better way of dealing with this.
Comment 22 Ryosuke Niwa 2010-09-08 23:04:46 PDT
Created attachment 66997 [details]
fixed per Darin's comments
Comment 23 Darin Adler 2010-09-09 11:02:51 PDT
(In reply to comment #21)
> > > +    // e.g. if pos was at Position("hello", 5) in <b>hello<div>world</div></b>, we want Position("world", 0) instead.
> > > +    // We only do this for range because caret at Position("hello", 5) in <b>hello</b>world should give you font-weight: bold.
> > > +    Node* posNode = pos.containerNode();
> > > +    if (selection()->isRange() && posNode && posNode->isTextNode() && pos.computeOffsetInContainerNode() == posNode->maxCharacterOffset())
> > > +        pos = nextVisuallyDistinctCandidate(pos);
> > 
> > Does the new test cover this case?
> 
> Yes, without this change, we'll get wrong font size for monospace case.
> 
> > I worry that this special case is too specific.
> 
> [...] As far as I'm concerned, the only way to distinguish this difference is to check whether the current selection is caret or not.

I was more worried about the way the rest of the test expression was written, not the isRange part. Specifically checking that the offset is the maximum offset within a text node. There’s an equivalent position just after the text node. Perhaps that’s taken care of by the “deep equivalent” concept?
Comment 24 Darin Adler 2010-09-09 11:05:29 PDT
Comment on attachment 66997 [details]
fixed per Darin's comments

> +    bool useFixedFontDefaultSize() const;

I’m glad you renamed this to be more specific. For the future, it’s not great to have functions that sound like they will take an action such as "use the fixed-font default size" that really mean the caller "should use the fixed-font default size". That’s why you’ll often see the word “should” in the name of boolean-returning functions. The word “use” makes the function be a verb phrase and sound like it will take action.
Comment 25 Ryosuke Niwa 2010-09-09 11:48:02 PDT
Thanks for the review.

(In reply to comment #23)
> (In reply to comment #21)
> > > I worry that this special case is too specific.
> > 
> > [...] As far as I'm concerned, the only way to distinguish this difference is to check whether the current selection is caret or not.
> 
> I was more worried about the way the rest of the test expression was written, not the isRange part. Specifically checking that the offset is the maximum offset within a text node. There’s an equivalent position just after the text node. Perhaps that’s taken care of by the “deep equivalent” concept?

Yes, pos can't be at an equivalent position right after the text node if we had canonicalized the position correctly.

(In reply to comment #24)
> (From update of attachment 66997 [details])
> > +    bool useFixedFontDefaultSize() const;
> 
> I’m glad you renamed this to be more specific. For the future, it’s not great to have functions that sound like they will take an action such as "use the fixed-font default size" that really mean the caller "should use the fixed-font default size". That’s why you’ll often see the word “should” in the name of boolean-returning functions. The word “use” makes the function be a verb phrase and sound like it will take action.

Sure.  I do agree in general.  But in this specific case, we're asking it to computed style, which represents the current style of an element obtained from its render style. I couldn't make semantic sense in asking whether nor not we "should" use fixed font default size.  Because after all, the render object is the one using a fixed font size, and we're asking the question of whether or not a fixed font size was used to render the object.  On the other hand, caller of this function is presumably using the value for something, and in that sense, "should" caller use or not makes sense.  At that point, I just decided to stick with useFixedFontDefaultSize() since it matches the function name in fontDescription.
Comment 26 Ryosuke Niwa 2010-09-09 11:55:27 PDT
Committed r67102: <http://trac.webkit.org/changeset/67102>
Comment 27 WebKit Review Bot 2010-09-09 12:17:42 PDT
http://trac.webkit.org/changeset/67102 might have broken Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/67101
http://trac.webkit.org/changeset/67102
Comment 28 Darin Adler 2010-09-09 14:29:36 PDT
(In reply to comment #25)
> I just decided to stick with useFixedFontDefaultSize() since it matches the function name in fontDescription.

Yes, I want to rename it in FontDescription and here too.
Comment 29 Darin Adler 2010-09-09 14:29:47 PDT
In the future.