Bug 21033

<rdar://problem/6240811>

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.

Created attachment 26942 [details] nicer test case

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.

This is causing Chromium Bug 7447.

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

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.

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.

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.

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

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

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.

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?

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

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

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

Created attachment 66948 [details] work in progress

Generally looks like a good approach. Typo: "lagacy".

Created attachment 66960 [details] Patch

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.

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

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

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

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.

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

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

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

In the future.