Bug 73546 - Add support for text auto-sizing.
: Add support for text auto-sizing.
Status: UNCONFIRMED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Alexander Færøy
:
Depends on: 56543
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-01 03:44 PST by Alexander Færøy
Modified: 2013-04-03 04:44 PDT (History)
18 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Kenneth Rohde Christiansen 2011-12-01 04:11:49 PST
You could provide links to the Microsoft and the Fennec implementation, test suites etc.
Comment 2 Alexander Færøy 2011-12-01 06:11:49 PST
Created attachment 117405 [details]
Initial patch.
Comment 3 Kenneth Rohde Christiansen 2011-12-01 06:33:22 PST
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
Comment 5 Simon Fraser (smfr) 2011-12-01 11:58:45 PST
Don't you need to fix bug 56543 first?
Comment 6 Yael 2012-01-21 18:14:49 PST
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.