[Webkit-unassigned] [Bug 73546] Add support for text auto-sizing.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=73546





--- Comment #3 from Kenneth Rohde Christiansen <kenneth at webkit.org>  2011-12-01 06:33:22 PST ---
(From update of attachment 117405)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.