WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED WONTFIX
74144
[CSS Regions] Elements in a region should be assignable to a named flow
https://bugs.webkit.org/show_bug.cgi?id=74144
Summary
[CSS Regions] Elements in a region should be assignable to a named flow
Alan Stearns
Reported
2011-12-08 17:08:25 PST
Created
attachment 118497
[details]
reftest It should be possible to redirect the children of an element that becomes a region to a different named flow. Currently this does not work - all of the region children are not displayed, even if there is flow-into and flow-from styling that should cause them to be displayed.
Attachments
reftest
(854 bytes, application/zip)
2011-12-08 17:08 PST
,
Alan Stearns
no flags
Details
patch
(31.91 KB, patch)
2013-04-02 07:03 PDT
,
Mihai Maerean
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from gce-cr-linux-03 for chromium-linux-x86_64
(935.72 KB, application/zip)
2013-04-02 08:35 PDT
,
WebKit Review Bot
no flags
Details
Archive of layout-test-results from gce-cr-linux-02 for chromium-linux-x86_64
(1.22 MB, application/zip)
2013-04-02 09:42 PDT
,
WebKit Review Bot
no flags
Details
patch that incorporates the feedback.
(24.63 KB, patch)
2013-04-05 02:09 PDT
,
Mihai Maerean
no flags
Details
Formatted Diff
Diff
patch that's 0.7-1.0% slower in the performance tests instead of 9.5%
(24.99 KB, patch)
2013-04-19 09:03 PDT
,
Mihai Maerean
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Vincent Hardy
Comment 1
2012-03-20 16:14:12 PDT
Submitted test to W3C:
https://hg.csswg.org/test/raw-file/d0faf2471ec7/contributors/adobe/incoming/css3-regions/regions-flow-descendants.xht
and
https://hg.csswg.org/test/raw-file/d0faf2471ec7/contributors/adobe/incoming/css3-regions/regions-flow-descendants-ref.xht
Mihai Balan
Comment 2
2013-02-21 10:02:59 PST
***
Bug 103685
has been marked as a duplicate of this bug. ***
Mihai Maerean
Comment 3
2013-04-02 07:03:02 PDT
Created
attachment 196131
[details]
patch
Stephen Chenney
Comment 4
2013-04-02 07:38:32 PDT
Comment on
attachment 196131
[details]
patch View in context:
https://bugs.webkit.org/attachment.cgi?id=196131&action=review
SVG bits seem fine. Can't comment on the rest.
> Source/WebCore/dom/Text.cpp:-305 > - if (!textRenderer || !textRendererIsNeeded(NodeRenderingContext(this, textRenderer->style()))) {
Why did you change this? It was cleaner before and it seems functionally equivalent.
Mihai Maerean
Comment 5
2013-04-02 07:52:15 PDT
Comment on
attachment 196131
[details]
patch View in context:
https://bugs.webkit.org/attachment.cgi?id=196131&action=review
>> Source/WebCore/dom/Text.cpp:-305 >> - if (!textRenderer || !textRendererIsNeeded(NodeRenderingContext(this, textRenderer->style()))) { > > Why did you change this? It was cleaner before and it seems functionally equivalent.
The previous code here threw a compile time error saying that "a non-const l-value NodeRenderingContext cannot be passed on a temporary object on the stack". I have thought about how to keep the old code as it clearly more elegant, but the compiler disagreed.
Andrei Bucur
Comment 6
2013-04-02 08:14:00 PDT
Comment on
attachment 196131
[details]
patch View in context:
https://bugs.webkit.org/attachment.cgi?id=196131&action=review
> LayoutTests/ChangeLog:8 > + * fast/regions/flow-body-in-html-expected.txt: Added.
Let's try to make use of check-layout.js in the tests. If not possible, ref tests are also a possibility. Layout text dumps are not portable between platforms and should be avoided.
> Source/WebCore/ChangeLog:8 > + DOM children of a region must not be rendered as children of the region, but can be collected in flow threads.
An overview of the approach taken to solve the bug would be helpful.
> Source/WebCore/dom/Text.cpp:305 > + if (!textRenderer) {
What changed here? Why the refactoring?
> Source/WebCore/rendering/RenderRegion.h:148 > + virtual bool canDOMChildrenHaveRenderParent() const OVERRIDE { return true; }
This is basically equivalent with isRenderRegion, right? Why do we need a second accessor? We want to cover isRenderRegionSet as well?
Mihai Maerean
Comment 7
2013-04-02 08:26:03 PDT
Comment on
attachment 196131
[details]
patch View in context:
https://bugs.webkit.org/attachment.cgi?id=196131&action=review
>> Source/WebCore/rendering/RenderRegion.h:148 >> + virtual bool canDOMChildrenHaveRenderParent() const OVERRIDE { return true; } > > This is basically equivalent with isRenderRegion, right? Why do we need a second accessor? We want to cover isRenderRegionSet as well?
This method ensures that this bug is fixed for good and other features in webkit can avoid it just by overriding the method. Using a virtual functions ensures that other features (newmulticol/overflow fragments) can decide their behavior without having to find the line of code in NodeRenderingContext.cpp where they would otherwise need to add code for themselves.
Mihnea Ovidenie
Comment 8
2013-04-02 08:28:37 PDT
Comment on
attachment 196131
[details]
patch View in context:
https://bugs.webkit.org/attachment.cgi?id=196131&action=review
> LayoutTests/ChangeLog:14 > +
For all the tests, you can use the dumpAsText() output. if (window.testRunner) testRunner.dumpAsText() In general, i would make all the tests for 74144, even those for the 103685. Specifying links to both bugs seems confusing to me.
> Source/WebCore/ChangeLog:9 > +
I would like to see a description of the general approach taken to solve this bug.
> Source/WebCore/ChangeLog:16 > + (WebCore):
I was told to remove the lines like the above from the Changelogs.
> Source/WebCore/dom/NodeRenderingContext.cpp:181 > + return 0;
This should follow the same pattern like: return m_renderingParent ? m_renderingParent->renderer() : 0;
WebKit Review Bot
Comment 9
2013-04-02 08:35:38 PDT
Comment on
attachment 196131
[details]
patch
Attachment 196131
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://webkit-commit-queue.appspot.com/results/17394078
New failing tests: fast/regions/flow-body-in-html.html fast/regions/universal-selector-children-to-the-same-region.html fast/regions/region-content-flown-into-region.html
WebKit Review Bot
Comment 10
2013-04-02 08:35:42 PDT
Created
attachment 196148
[details]
Archive of layout-test-results from gce-cr-linux-03 for chromium-linux-x86_64 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-03 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Mihnea Ovidenie
Comment 11
2013-04-02 09:00:21 PDT
Comment on
attachment 196131
[details]
patch View in context:
https://bugs.webkit.org/attachment.cgi?id=196131&action=review
> Source/WebCore/dom/NodeRenderingContext.cpp:173 > + if (m_node->isElementNode() && toElement(m_node)->moveToFlowThreadIsNeeded()) {
Is there a way to cache the result of calling styleForRenderer() for m_node and pass it to moveToFlowThreadIsNeeded() and also initialize m_style member in this class? You are calling styleForRenderer() a lot and i think you can reduce the necessary calls.
WebKit Review Bot
Comment 12
2013-04-02 09:42:52 PDT
Comment on
attachment 196131
[details]
patch
Attachment 196131
[details]
did not pass chromium-ews (chromium-xvfb): Output:
http://webkit-commit-queue.appspot.com/results/17240831
New failing tests: fast/regions/flow-body-in-html.html fast/regions/universal-selector-children-to-the-same-region.html fast/regions/region-content-flown-into-region.html
WebKit Review Bot
Comment 13
2013-04-02 09:42:57 PDT
Created
attachment 196167
[details]
Archive of layout-test-results from gce-cr-linux-02 for chromium-linux-x86_64 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Mihai Maerean
Comment 14
2013-04-04 06:03:51 PDT
Comment on
attachment 196131
[details]
patch View in context:
https://bugs.webkit.org/attachment.cgi?id=196131&action=review
> Source/WebCore/dom/Element.cpp:2573 > + // FIXME: Do not collect elements if they are in shadow tree. > if (isInShadowTree()) > return false;
I have carried this FIXME text along but it seems the code below fixes the problem. I will remove this FIXME comment. Please comment if you think otherwise.
Mihai Maerean
Comment 15
2013-04-05 02:09:34 PDT
Created
attachment 196601
[details]
patch that incorporates the feedback.
Mihnea Ovidenie
Comment 16
2013-04-05 09:21:22 PDT
(In reply to
comment #15
)
> Created an attachment (id=196601) [details] > patch that incorporates the feedback.
Ok, we need David Hyatt's input on this.
Dave Hyatt
Comment 17
2013-04-05 09:25:59 PDT
Comment on
attachment 196601
[details]
patch that incorporates the feedback. r=me
WebKit Commit Bot
Comment 18
2013-04-05 10:13:33 PDT
Comment on
attachment 196601
[details]
patch that incorporates the feedback. Clearing flags on attachment: 196601 Committed
r147756
: <
http://trac.webkit.org/changeset/147756
>
WebKit Commit Bot
Comment 19
2013-04-05 10:13:36 PDT
All reviewed patches have been landed. Closing bug.
Thiago Marcos P. Santos
Comment 20
2013-04-08 08:52:19 PDT
This patch is causing the performance regression we can see today on the performance bots. On my tests, I could measure a slowdown of 9.5% on the Parser/html5-full-render.html test. These are my runs (ms, lower is better):
r147911
4642.6008261 4641.89847605 4643.60051905
r147911
(with this patch reverted) 4239.35488146 4234.28493692 4251.07173051 For the record: This is the shortest range of "suspects" by crossing the information from the performance bots.
http://trac.webkit.org/log/?rev=147758&stop_rev=147752&verbose=on
Mihai Maerean
Comment 21
2013-04-08 23:01:25 PDT
I'm rolling out the patch as part of
https://bugs.webkit.org/show_bug.cgi?id=114176
Mihai Maerean
Comment 22
2013-04-19 09:03:42 PDT
Created
attachment 198890
[details]
patch that's 0.7-1.0% slower in the performance tests instead of 9.5% Running the performance tests on 2 machines gets only 0.7-1.0% bigger times instead of the 9.5% of the previous patch. The big performance impact of the previous patch was caused by always setting m_style = element->styleForRenderer() in NodeRenderingContext::createRendererForElementIfNeeded() even if the style was already computed and stored in m_style.
Dave Hyatt
Comment 23
2013-04-19 11:56:13 PDT
Comment on
attachment 198890
[details]
patch that's 0.7-1.0% slower in the performance tests instead of 9.5% r=me
WebKit Commit Bot
Comment 24
2013-04-22 02:04:42 PDT
Comment on
attachment 198890
[details]
patch that's 0.7-1.0% slower in the performance tests instead of 9.5% Clearing flags on attachment: 198890 Committed
r148865
: <
http://trac.webkit.org/changeset/148865
>
WebKit Commit Bot
Comment 25
2013-04-22 02:04:49 PDT
All reviewed patches have been landed. Closing bug.
Simon Fraser (smfr)
Comment 26
2013-05-28 22:57:28 PDT
The extra styleForRenderer() that this added in Element::moveToFlowThreadIsNeeded() is showing up on profiles. This code needs to be conditionalized to never run on pages that don't use regions.
Simon Fraser (smfr)
Comment 27
2013-05-28 22:59:23 PDT
Maybe RenderView::flowThreadController() should return nil until a named flow is actually encountered, and that can be used as the trigger to hit slow code.
Mihai Maerean
Comment 28
2013-05-29 07:47:24 PDT
(In reply to
comment #26
)
> The extra styleForRenderer() that this added in Element::moveToFlowThreadIsNeeded() is showing up on profiles. This code needs to be conditionalized to never run on pages that don't use regions.
When parsing a page without regions you need to compute the style to read the "flow" properties to know whether the page has regions. So this is a logical loop. In short, how do we detect that the page doesn't use regions without computing the style (note that NodeRenderingContext is used when parsing the page)?
Simon Fraser (smfr)
Comment 29
2013-05-29 09:16:29 PDT
Why does moveToFlowThreadIsNeeded() need to every compute style itself? When does the renderer not have a style?
Mihai Maerean
Comment 30
2013-06-07 08:50:16 PDT
I've been actively discussing this problem at
https://codereview.chromium.org/15027005/
with Julien Chaffraix. When the discussion comes to a conclusion I will post the conclussions here too (and a patch).
Mihai Maerean
Comment 31
2013-06-14 06:01:24 PDT
As Simon Fraser suggested that there could be less calls to Element::styleForRenderer(), I'm working on it at
https://bugs.webkit.org/show_bug.cgi?id=117637
Mihai Maerean
Comment 32
2013-07-15 04:42:43 PDT
Closing the bug, as Simon's concerns are not applicable on the latest code.
Mihnea Ovidenie
Comment 33
2014-04-04 00:48:25 PDT
Reopened after
http://trac.webkit.org/changeset/166353
temporarily removed support for this. Changeset
http://trac.webkit.org/changeset/166632
added a test for that.
Brent Fulgham
Comment 34
2022-07-13 09:28:47 PDT
CSS Regions were removed in
Bug 174978
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug