Bug 94472 – [CSS Regions] Add support for text-shadow in region styling

• Status: RESOLVED
• Resolution: WONTFIX
• Component: CSS

[Andrei Onea]

The spec permits region styling to be applied for the text-shadow property. We need to also implement this in WebKit.

[Andrei Onea]

Created attachment 159431 [details]
Patch

[WebKit Review Bot]

Comment on attachment 159431 [details]
Patch

Attachment 159431 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13544375

New failing tests:
fast/regions/region-style-text-shadow.html

[WebKit Review Bot]

Created attachment 159441 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Julien Chaffraix]

Comment on attachment 159431 [details]
Patch

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

> Source/WebCore/rendering/InlineFlowBox.cpp:165
> +                childStyle = flowThread->renderStyleForLine(this);

Won't this break :first-letter or :first-line in region as you don't pass in isFirstLineStyle()?

[Julien Chaffraix]

I am no expert in line box layout so CC'ing people who know this code better in case they have some comment.

[Dave Hyatt]

Comment on attachment 159431 [details]
Patch

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

r- (Comments below)

> Source/WebCore/rendering/InlineFlowBox.cpp:166
>              RenderStyle* childStyle = child->renderer()->style(isFirstLineStyle());
> +            if (renderer()->inRenderFlowThread()) {
> +                RenderFlowThread* flowThread = renderer()->enclosingRenderFlowThread();
> +                childStyle = flowThread->renderStyleForLine(this);
> +            }

I would bundle all of this together into a styleInRegion() function on InlineBox. Then the code here would just read:

RenderStyle* childStyle = child->styleInRegion(child->region(), isFirstLineStyle());

Technically if the first line style bit is properly set on the child box, you don't even need to pass in isFirstLineStyle().

In other words, let's separate fetching the style for a specific region from the function that figures out what region the child is in. I'll explain why below.

> Source/WebCore/rendering/InlineFlowBox.cpp:832
>      RenderStyle* style = textBox->renderer()->style(isFirstLineStyle());
>      
> +    if (renderer()->inRenderFlowThread()) {
> +        RenderFlowThread* flowThread = renderer()->enclosingRenderFlowThread();
> +        style = flowThread->renderStyleForLine(this);
> +    }

Same here. All of this could just be inside InlineBox helpers, since you just did the exact same thing as above.

> Source/WebCore/rendering/RenderFlowThread.cpp:405
> +    LayoutUnit offsetFromLogicalTop = toRenderBlock(renderer)->offsetFromLogicalTopOfFirstPage();
> +    if (flowBox->prevLineBox())
> +        offsetFromLogicalTop += flowBox->prevLineBox()->logicalTop();

This is not correct. You're treating the first line as though it started right at the very top of the RenderBlock, but there could be border/padding/floats, etc., any # of things that push even the first line down. Why aren't you just adding in your own logicalTop? I don't understand why the previous line box has to be involved at all.

Also, this is very fragile code, since it only works during layout. We don't know the offsetFromLogicalTopOfFirstPage at paint time for example (we'll have the RenderRegion in the painting info in that case).

This is why I'd like to see styleInRegion() and region() as two different helpers on InlineBox. Then at paint time, you'll be able to still ask for the region-specific style and just pass in the RenderRegion from the painting info (since you won't be able to rely on offsetFromLogicalTopOfFirstPage).

> Source/WebCore/rendering/RenderFlowThread.cpp:409
> +    return region->computeStyleInRegion(renderer).get();

Where is first line styling taken into account? It looks like it's being dropped on the floor.

[Andrei Onea]

Created attachment 160697 [details]
Patch

[WebKit Review Bot]

Comment on attachment 160697 [details]
Patch

Attachment 160697 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13622078

New failing tests:
fast/regions/region-style-text-shadow.html

[WebKit Review Bot]

Created attachment 160703 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Dave Hyatt]

Comment on attachment 160697 [details]
Patch

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

Looking good! Just a few suggestions.

> Source/WebCore/rendering/InlineBox.cpp:257
> +RenderStyle* InlineBox::styleInRegion(RenderRegion* region, bool isFirstLine)

You don't need the second argument. The style bit has been set on the box itself at all the call sites I see you using this function. So you can just use the member on InlineBox.

> Source/WebCore/rendering/InlineBox.cpp:268
> +RenderRegion* InlineBox::region()

Let's rename this to "regionDuringLayout()" to make it clear you can't call it at other times.

> Source/WebCore/rendering/InlineBox.cpp:274
> +    return flowThread->renderRegionForLine(renderer()->containingBlock()->offsetFromLogicalTopOfFirstPage());

You should add in the logicalHeight() of the containing block, so that you're at least at the bottom of the previous line.

> Source/WebCore/rendering/InlineBox.h:103
> +    RenderRegion* region();

regionDuringLayout() rename. Also let's add a comment that indicates that this assumes the box has not been positioned yet, and that it will use the block's current logical height (similar to what floats do).

[Dave Hyatt]

Let's get some layout tests where the text-shadow is on a later line in the block and not just at the top.

[Andrei Onea]

Created attachment 161028 [details]
Patch

[WebKit Review Bot]

Comment on attachment 161028 [details]
Patch

Attachment 161028 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13651576

New failing tests:
fast/regions/region-style-text-shadow.html

[WebKit Review Bot]

Created attachment 161098 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Mihai Balan]

Comment on attachment 161028 [details]
Patch

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

> LayoutTests/fast/regions/region-style-text-shadow-expected.html:10
> +             #p1 { text-shadow: 340px 100px #008000; position: absolute; top: 10px; }

Why are you manually positioning every element absolutely? It really makes the test a lot harder to read/interpret

> LayoutTests/fast/regions/region-style-text-shadow.html:14
> +             #region1 { -webkit-flow-from: flow1; position: absolute; top: 10px; }

Is there a particular reason why you manually specify the position of the regions? I would like the tests as clear and clutter free as possible :)

> LayoutTests/fast/regions/region-style-text-shadow.html:59
> +         <div id="article1">

For the sake of maintainability / readability, I would rather have one text at the bottom of the file explaining the expected output (E.g. "No red shadow should be visible") and then each "assertion" to better explain what exactly is it testing rather than having "Text shadow styled in region: <color>" over and over again :)

> LayoutTests/fast/regions/region-style-text-shadow.html:87
> +         <div id="article5">

Aren't article5 and article6 testing the same thing? (The only difference that I see is in the element used inside the <article> tag. Why do you think they would behave differently?

[Andrei Onea]

Created attachment 161247 [details]
Patch

[Dave Hyatt]

Comment on attachment 161247 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 161247 [details]
Patch

Rejecting attachment 161247 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ineTextBox.o
Source/WebCore/rendering/InlineBox.cpp: In member function 'WebCore::RenderRegion* WebCore::InlineBox::regionDuringLayout()':
Source/WebCore/rendering/InlineBox.cpp:278: error: 'class WebCore::RenderFlowThread' has no member named 'renderRegionForLine'
  CXX(target) out/Release/obj.target/webcore_rendering/Source/WebCore/rendering/LayoutState.o
make: *** [out/Release/obj.target/webcore_rendering/Source/WebCore/rendering/InlineBox.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/13682416

[Andrei Onea]

Created attachment 161437 [details]
Patch for landing

[EFL EWS Bot]

Comment on attachment 161437 [details]
Patch for landing

Attachment 161437 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13682635

[EFL EWS Bot]

(In reply to comment #20)
> (From update of attachment 161437 [details])
> Attachment 161437 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/13682635

This is false alarm. Please ignore it. Sorry for noise.

[WebKit Review Bot]

Comment on attachment 161437 [details]
Patch for landing

Clearing flags on attachment: 161437

Committed r127131: <http://trac.webkit.org/changeset/127131>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 95463

[Julien Chaffraix]

Here is a crash report running valgrind on Chromium linux: valgrind out/Debug/DumpRenderTree --no-timeout LayoutTests/fast/regions/bottom-overflow-out-of-first-region.html

==7220== Memcheck, a memory error detector
==7220== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.
==7220== Using Valgrind-3.6.0.SVN-Debian and LibVEX; rerun with -h for copyright info
==7220== Command: out/Debug/DumpRenderTree --no-timeout LayoutTests/fast/regions/bottom-overflow-out-of-first-region.html
==7220==
==7220== Warning: set address range perms: large range [0x3815cfafb000, 0x3815efafb000) (noaccess)
==7220== Invalid read of size 2
==7220==    at 0x520E46: WebCore::Font::letterSpacing() const (Font.h:117)
==7220==    by 0x1620D2B: WebCore::RenderStyle::letterSpacing() const (RenderStyle.cpp:1210)
==7220==    by 0x142AFDF: WebCore::InlineFlowBox::addToLine(WebCore::InlineBox*) (InlineFlowBox.cpp:163)
==7220==    by 0x1495818: WebCore::RenderBlock::createLineBoxes(WebCore::RenderObject*, WebCore::LineInfo const&, WebCore::InlineBox*) (RenderBlockLineLayout.cpp:487)
==7220==    by 0x1495BDD: WebCore::RenderBlock::constructLine(WebCore::BidiRunList<WebCore::BidiRun>&, WebCore::LineInfo const&) (RenderBlockLineLayout.cpp:556)
==7220==    by 0x14983E6: WebCore::RenderBlock::createLineBoxesFromBidiRuns(WebCore::BidiRunList<WebCore::BidiRun>&, WebCore::InlineIterator const&, WebCore::LineInfo&, WebCore::VerticalPositionCache&, WebCore::BidiRun*) (RenderBlockLineLayout.cpp:1078)
==7220==    by 0x149950B: WebCore::RenderBlock::layoutRunsAndFloatsInRange(WebCore::LineLayoutState&, WebCore::BidiResolver<WebCore::InlineIterator, WebCore::BidiRun>&, WebCore::InlineIterator const&, WebCore::BidiStatus const&, unsigned int) (RenderBlockLineLayout.cpp:1370)
==7220==    by 0x1498AB8: WebCore::RenderBlock::layoutRunsAndFloats(WebCore::LineLayoutState&, bool) (RenderBlockLineLayout.cpp:1271)
==7220==    by 0x149AF2D: WebCore::RenderBlock::layoutInlineChildren(bool, WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&) (RenderBlockLineLayout.cpp:1600)
==7220==    by 0x144A99D: WebCore::RenderBlock::layoutBlock(bool, WebCore::FractionalLayoutUnit) (RenderBlock.cpp:1531)
==7220==    by 0x1449ED7: WebCore::RenderBlock::layout() (RenderBlock.cpp:1374)
==7220==    by 0x144FAA1: WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&) (RenderBlock.cpp:2452)
==7220==  Address 0x363636363636369e is not stack'd, malloc'd or (recently) free'd
==7220==
[7220:7220:1477808315938:ERROR:process_util_posix.cc(143)] Received signal 11
        base::debug::StackTrace::StackTrace() [0x1419e1a]
        base::(anonymous namespace)::StackDumpSignalHandler() [0x13c68b5]
        0xccccaf0
        WebCore::Font::letterSpacing() [0x520e46]
        WebCore::RenderStyle::letterSpacing() [0x1620d2c]
        WebCore::InlineFlowBox::addToLine() [0x142afe0]
        WebCore::RenderBlock::createLineBoxes() [0x1495819]
        WebCore::RenderBlock::constructLine() [0x1495bde]
        WebCore::RenderBlock::createLineBoxesFromBidiRuns() [0x14983e7]
        WebCore::RenderBlock::layoutRunsAndFloatsInRange() [0x149950c]
        WebCore::RenderBlock::layoutRunsAndFloats() [0x1498ab9]
        WebCore::RenderBlock::layoutInlineChildren() [0x149af2e]
        WebCore::RenderBlock::layoutBlock() [0x144a99e] 
        WebCore::RenderBlock::layout() [0x1449ed8]
        WebCore::RenderBlock::layoutBlockChild() [0x144faa2]
        WebCore::RenderBlock::layoutBlockChildren() [0x144f619]
        WebCore::RenderBlock::layoutBlock() [0x144a9bf] 
        WebCore::RenderBlock::layout() [0x1449ed8]
        WebCore::RenderBlock::layoutBlockChild() [0x144faa2]
        WebCore::RenderBlock::layoutBlockChildren() [0x144f619]
        WebCore::RenderBlock::layoutBlock() [0x144a9bf] 
        WebCore::RenderBlock::layout() [0x1449ed8]
        WebCore::RenderBlock::layoutBlockChild() [0x144faa2]
        WebCore::RenderBlock::layoutBlockChildren() [0x144f619]
        WebCore::RenderBlock::layoutBlock() [0x144a9bf] 
        WebCore::RenderBlock::layout() [0x1449ed8]
        WebCore::RenderBlock::layoutBlockChild() [0x144faa2]
        WebCore::RenderBlock::layoutBlockChildren() [0x144f619]
        WebCore::RenderBlock::layoutBlock() [0x144a9bf] 
        WebCore::RenderBlock::layout() [0x1449ed8]
        WebCore::RenderFlowThread::layout() [0x1503a88] 
        WebCore::RenderObject::layoutIfNeeded() [0x141e45b]
        WebCore::FlowThreadController::layoutRenderNamedFlowThreads() [0x141dfee]
        WebCore::RenderView::layout() [0x15f79ee]
        WebCore::FrameView::layout() [0x2493160]
        WebCore::FrameView::layoutTimerFired() [0x2496a23]
        WebCore::Timer<>::fired() [0x24a39ee]
        WebCore::ThreadTimers::sharedTimerFiredInternal() [0x63aa10]

Here is the crash under gdb:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000520e46 in WebCore::Font::letterSpacing (this=0x363636363636366e) at ../../Source/WebCore/platform/graphics/Font.h:117
117         short letterSpacing() const { return m_letterSpacing; }
(gdb) f 1
#1  0x0000000001620d2c in WebCore::RenderStyle::letterSpacing (this=0x7fffeb36dd80) at ../../Source/WebCore/rendering/style/RenderStyle.cpp:1210
1210    int RenderStyle::letterSpacing() const { return inherited->font.letterSpacing(); }

[Mihnea Ovidenie]

This caused also a crash that can be seen in https://bugs.webkit.org/show_bug.cgi?id=95583

[Andrei Onea]

Created attachment 163610 [details]
Patch

[Dave Hyatt]

Comment on attachment 163610 [details]
Patch

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

> Source/WebCore/rendering/InlineFlowBox.cpp:163
> -            RenderStyle* childStyle = child->renderer()->style(isFirstLineStyle());
> +            RefPtr<RenderStyle> childStyle = child->styleInRegion(child->regionDuringLayout());

Let's follow this through to its logical conclusion, which is that all style access during layout that needs to get a style that can vary from region to region is going to have to be put in a refptr just to keep it from going away? Ick. If someone repeatedly asks for style, they're going to cause it to be computed again and again.

It seems obvious that we have to change the architecture here for what to do with region styling during layout. My opinion is that the styles should be cached and updated once they are accessed once and they should stick around as long as the region lives (and as long as the renderer that the style is applying to lives). If you had a cache like that it could work for both layout and painting.

Computing the styles and then tossing them for painting is not exactly a great approach either, since you do way more work when painting than you need to.

I think ultimately there's going to need to be a method like styleAtOffset that layout / painting use to fetch the appropriate region-specific style for an object. If the style doesn't exist, then it could be created at that time.

I have no problem with a first-cut cache destroying all render styles at the end of every layout (similar to what painting does today). If you want to take that approach initially of doing something kind of like what painting does, I'd be cool with that.

[Andrei Onea]

Created attachment 164572 [details]
Patch

[Mihnea Ovidenie]

Comment on attachment 164572 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        (WebCore::InlineBox::styleInRegion):

I would like to see a comment describing what this function is doing.

> Source/WebCore/ChangeLog:19
> +        (WebCore::InlineBox::regionDuringLayout):

Same here with the mention that this function should be used only during layout.

> Source/WebCore/ChangeLog:44
> +        value and caches it.

Since this function is added in this patch, i would change the comment to something like: "This function exposes region styling information to  be available not only at paint time, but also at layout time".

> Source/WebCore/rendering/InlineBox.cpp:272
> +    // This assumes that the box has not been positioned yet, so it uses the containing block's current logical height to get the region.

Is there a way to test this assumption?

> Source/WebCore/rendering/InlineBox.h:103
> +    RenderRegion* regionDuringLayout();

These 2 methods can be made const.

> Source/WebCore/rendering/RenderRegion.cpp:-466
> -    ASSERT(!object->isAnonymous());

An explanation in the changelog about this assertion removal would be nice.

> Source/WebCore/rendering/RenderRegion.cpp:463
> +    RenderStyle* defaultParentStyle = object->parent() && !object->node()->inNamedFlow()? ensureRegionStyleForObject(object->parent()):0;

I would also add a line in the changelog about the computation (and caching) of parent style in region.

> LayoutTests/fast/regions/region-style-text-shadow-expected.html:10
> +             #p1 { text-shadow: 340px 100px #008000; position: absolute; top: 10px; }

In this test, you have 2 text-shadow styles that you are using. I would create 2 classes for them and annotate the elements with them.

[Andrei Onea]

Created attachment 164736 [details]
Patch

[Mihnea Ovidenie]

Created attachment 171384 [details]
Patch 8

Bring the patch up-to-date.

[WebKit Review Bot]

Attachment 171384 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/rendering/RenderRegion.cpp:513:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mihnea Ovidenie]

Created attachment 171387 [details]
Patch 9

[Dave Hyatt]

Comment on attachment 171387 [details]
Patch 9

r=me

[Mihnea Ovidenie]

Created attachment 172282 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 172282 [details]
Patch for landing

Rejecting attachment 172282 [details] from commit-queue.

New failing tests:
fast/regions/region-style-inline-background-color.html
Full output: http://queues.webkit.org/results/14712957

[Mihnea Ovidenie]

Created attachment 173226 [details]
Patch for landing

[Mihnea Ovidenie]

I have build chromium port with the patch applied on Linux and the failing test passed. I have also built Chromium on Linux with the patch and run the test manually without noticing a problem. I decided to try to apply the patch once again. I wish we would have a way to see the errors in such cases.

[WebKit Review Bot]

Comment on attachment 173226 [details]
Patch for landing

Rejecting attachment 173226 [details] from commit-queue.

New failing tests:
fast/regions/region-style-inline-background-color.html
Full output: http://queues.webkit.org/results/14781227

[Mihnea Ovidenie]

Created attachment 173608 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 173608 [details]
Patch for landing

Clearing flags on attachment: 173608

Committed r134205: <http://trac.webkit.org/changeset/134205>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[János Badics]

After http://trac.webkit.org/changeset/134205, fast/regions/region-style-inline-background-color.html and fast/regions/webkit-flow-inlines-dynamic.html fail on Qt (the second one fails on GTK and EFL, too). You can find details at https://bugs.webkit.org/show_bug.cgi?id=101933

[WebKit Review Bot]

Re-opened since this is blocked by bug 102090

[Brent Fulgham]

CSS Regions were removed in Bug 174978.