Bug 21033 – queryCommandValue('FontSize') returns pixel values instead of IE font numbers

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

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)

[Mark Rowe (bdash)]

<rdar://problem/6240811>

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Created attachment 26942 [details]
nicer test case

[Eric Seidel (no email)]

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.

[Jon@Chromium]

This is causing Chromium Bug 7447.

[Jon@Chromium]

*** Bug 24855 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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. :)

[Jon@Chromium]

Eric, I would really like to have this fixed for the 3.0 release of Chrome.

[Jon@Chromium]

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.

[Julie Parent]

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?

[Jon@Chromium]

Yes, this is a lower priority that the one for which there is no workaround.

[Eric Seidel (no email)]

My test case broke after changes to execCommand to have it remove uneeded spans.  Will fix.

[Eric Seidel (no email)]

Created attachment 33663 [details]
updated test case, including results affected by bug 19161

[Ryosuke Niwa]

Created attachment 66948 [details]
work in progress

[Darin Adler]

Generally looks like a good approach.

Typo: "lagacy".

[Ryosuke Niwa]

Created attachment 66960 [details]
Patch

[Darin Adler]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 66997 [details]
fixed per Darin's comments

[Darin Adler]

(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?

[Darin Adler]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Committed r67102: <http://trac.webkit.org/changeset/67102>

[WebKit Review Bot]

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

[Darin Adler]

(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.

[Darin Adler]

In the future.