Bug 73546 – Add support for text auto-sizing.

• Component: WebCore Misc.
• Status: RESOLVED DUPLICATE

[Alexander Færøy]

Add support for text auto-sizing as specified here:

http://developer.apple.com/library/safari/#documentation/appleapplications/reference/safariwebcontent/AdjustingtheTextSize/AdjustingtheTextSize.html

[Kenneth Rohde Christiansen]

You could provide links to the Microsoft and the Fennec implementation, test suites etc.

[Alexander Færøy]

Created attachment 117405 [details]
Initial patch.

[Kenneth Rohde Christiansen]

Comment on attachment 117405 [details]
Initial patch.

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

Please run prepare-Changelog and start to explain the changes and give some credit, like mention that the original code is from iOS but modified by at least me and you. Also, info about both MS and fennec supporting it would be great to add.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1737
> -            if (style->textSizeAdjust())
> +            if (style->textSizeAdjust().isAuto())
>                  return primitiveValueCache->createIdentifierValue(CSSValueAuto);
> -            return primitiveValueCache->createIdentifierValue(CSSValueNone);
> +            if (style->textSizeAdjust().isNone())
> +                return primitiveValueCache->createIdentifierValue(CSSValueNone);
> +
> +            return CSSPrimitiveValue::create(style->textSizeAdjust().percentage(), CSSPrimitiveValue::CSS_PERCENTAGE);

You need to run the test suite before a change like then and after. Maybe it would be possible to split up parts of the patch.

> Source/WebCore/css/CSSStyleSelector.cpp:3491
> +#if 0
>          setTextSizeAdjust(primitiveValue->getIdent() == CSSValueAuto);
> +#endif

Why is this outcommented?

> Source/WebCore/dom/Document.cpp:4679
> +    // FIXME: Build, was these removed?
> +    // if (currentStyle->affectedByAttributeSelectors())
> +    //    newStyle->setAffectedByAttributeSelectors();

I would talk to Andreas Kling and Antti Koivisto. You could cc them. Also cc Laszlo as he has tests for the feature added by this patch

> Source/WebCore/dom/Document.cpp:4801
> +    // If we only have one piece of text with the style on the page, do not adjust its size.
> +    if (m_autoSizedNodes.size() <= 1)
> +        return;

This needs a test

> Source/WebCore/dom/Document.cpp:4955
> +        RefPtr<TextAutoSizingValue> value = i->second;
> +        if (value)
> +            value->reset();

I guess these could be merged to two lines

> Source/WebCore/dom/Document.h:1191
> +    void addAutoSizingNode(Node *node, float size);

coding style. Should be (Node*, float size)

> Source/WebCore/page/FrameView.cpp:1105
> +    // FIXME: Create setting instead, like setMinimumZoomedToElementFontSize().
> +    float minimumZoomedToElementFontSize = 16;
> +    float visibleWidth = 320; // FIXME: Verify whether this is correct.
> +    if (minimumZoomedToElementFontSize && visibleWidth && !root->view()->printing()) {
> +        root->adjustComputedFontSizesOnBlocks(minimumZoomedToElementFontSize, visibleWidth);
> +        if (root->needsLayout())
> +            root->layout();
> +    }

This is not upstreamable... It needs to be fixed. Also indentation is off

> Source/WebCore/rendering/RenderBlock.cpp:6871
> +    // We disallow resizing for text input fields and textarea to address <rdar://problem/5792987> and <rdar://problem/8021123>

Test needed, though we need to deduct the issue here. This could/should be a separate patch.

> Source/WebCore/rendering/RenderBlock.cpp:6924
> +    // Don't do any work if the block is smaller than the visible area.
> +    if (visibleWidth >= width())
> +        return;

This would also need a test

> Source/WebCore/rendering/RenderBlock.cpp:6932
> +            lineCount = NO_LINE;

I don't like the variable name here, as it is not exactly a count, it is more like a type. Suggestions?

> Source/WebCore/rendering/RenderBlock.h:65
> +enum LineCount {
> +    NOT_SET = 0,
> +    NO_LINE = 1,
> +    ONE_LINE = 2,
> +    MULTI_LINE = 3
> +};

We probably want to change this. Also it doesn't seem to be aligned with our style

> Source/WebCore/rendering/RenderBlock.h:305
> +        m_lineCountForTextAutosizing = NOT_SET;

m_lineCountForTextAutosizing should probably also be renamed

> Source/WebCore/rendering/RenderObject.cpp:600
> +    // We don't apply autosizing to nodes with fixed height normally.
> +    // But we apply it to nodes which are located deep enough
> +    // (nesting depth is greater than some const) inside of a parent block
> +    // which has fixed height but its content overflows intentionally.

Needs a test. Could be removed from initial patch. Maybe you could split this patch up in multiple patches

> Source/WebCore/rendering/RenderObject.h:197
> +    // Minimal distance between the block with fixed height and overflowing content and the text block to apply text autosizing.
> +    // The greater this constant is the more potential places we have where autosizing is turned off.
> +    // So it should be as low as possible. There are sites that break at 2.
> +    static const int TextAutoSizingFixedHeightDepth = 3;
> +
> +    enum BlockContentHeightType {
> +        FixedHeight,
> +        FlexibleHeight,
> +        OverflowHeight
> +    };

All this depth thing, is really a separate patch, with separate tests. It would also make the initial patch a lot smaller

[Kenneth Rohde Christiansen]

Related resources:

http://msdn.microsoft.com/en-us/library/ff462082(v=vs.92).aspx
http://blogs.msdn.com/b/iemobile/archive/2010/05/10/javascript-and-css-changes-in-ie-mobile-for-windows-phone-7.aspx
https://github.com/necolas/normalize.css/issues/28
http://webdesignernotebook.com/css/the-little-known-font-size-adjust-css3-property/
http://dbaron.org/log/20111126-font-inflation

[Simon Fraser (smfr)]

Don't you need to fix bug 56543 first?

[Yael]

Comment on attachment 117405 [details]
Initial patch.

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

> Source/WebCore/dom/Document.cpp:4866
> +            RefPtr<RenderStyle> newParentStyle = cloneRenderStyleWithState(parentStyle);
> +            newParentStyle->setLineHeight(Length(lineHeight, Fixed));
> +            newParentStyle->setSpecifiedLineHeight(lineHeightLength);
> +            newParentStyle->setFontDescription(fontDescription);
> +            newParentStyle->font().update(autoSizingNode->document()->styleSelector()->fontSelector());
> +            parentRenderer->setStyle(newParentStyle.release());

This will cause you to loose visitedStyle if this is a visited link.
You should copy the visitedStyle too, here and in other places that clone and change the style.

[Daniel Bates]

*** This bug has been marked as a duplicate of bug 121111 ***