Bug 73546 - Add support for text auto-sizing.
: Add support for text auto-sizing.
Status: UNCONFIRMED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 56543
:
  Show dependency treegraph
 
Reported: 2011-12-01 03:44 PST by
Modified: 2013-04-03 04:44 PST (History)


Attachments
Initial patch. (53.96 KB, patch)
2011-12-01 06:11 PST, Alexander Færøy
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


------- Comment #1 From 2011-12-01 04:11:49 PST -------
You could provide links to the Microsoft and the Fennec implementation, test suites etc.
------- Comment #2 From 2011-12-01 06:11:49 PST -------
Created an attachment (id=117405) [details]
Initial patch.
------- Comment #3 From 2011-12-01 06:33:22 PST -------
(From update of attachment 117405 [details])
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
------- Comment #5 From 2011-12-01 11:58:45 PST -------
Don't you need to fix bug 56543 first?
------- Comment #6 From 2012-01-21 18:14:49 PST -------
(From update of attachment 117405 [details])
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.