Bug 199049 – Preview of <picture> element doesn't match element bounds

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

Preview of <picture> element doesn't match element bounds

[Tim Horton]

Created attachment 372509 [details]
Patch

[Tim Horton]

<rdar://problem/51474402>

[Tim Horton]

*** Bug 199048 has been marked as a duplicate of this bug. ***

[EWS Watchlist]

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


ERROR: Source/WebCore/testing/Internals.h:844:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/testing/Internals.h:852:  The parameter name "range" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/testing/Internals.h:852:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 12 files


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

[EWS Watchlist]

Comment on attachment 372509 [details]
Patch

Attachment 372509 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12526147

New failing tests:
fast/text-indicator/text-indicator-uses-img-size-inside-picture.html

[EWS Watchlist]

Created attachment 372515 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 372509 [details]
Patch

Attachment 372509 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12526187

New failing tests:
fast/text-indicator/text-indicator-uses-img-size-inside-picture.html

[EWS Watchlist]

Created attachment 372517 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 372509 [details]
Patch

Attachment 372509 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12526335

New failing tests:
fast/text-indicator/text-indicator-uses-img-size-inside-picture.html

[EWS Watchlist]

Created attachment 372520 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[EWS Watchlist]

Comment on attachment 372509 [details]
Patch

Attachment 372509 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12526342

New failing tests:
fast/text-indicator/text-indicator-uses-img-size-inside-picture.html

[EWS Watchlist]

Created attachment 372521 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 372509 [details]
Patch

Attachment 372509 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12526437

New failing tests:
fast/text-indicator/text-indicator-uses-img-size-inside-picture.html

[EWS Watchlist]

Created attachment 372522 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.5

[Simon Fraser (smfr)]

Comment on attachment 372509 [details]
Patch

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

> Source/WebCore/dom/Range.cpp:1824
> +                    if (containingBlock.hasOverflowClip() && (style.overflowX() != Overflow::Visible || style.overflowY() != Overflow::Visible)) {

Maybe just containingBlock.hasOverflowClip()?

> Source/WebCore/dom/Range.cpp:1839
> +                    auto bounding = quad.boundingBox();

bounding is what springboks do

> Source/WebCore/dom/Range.h:119
> +    enum class RectangleGenerationBehavior : uint8_t {

BoundingRectBehavior?

> Source/WebCore/dom/Range.h:121
> +        IncludeRectsForDeepElements = 1 << 1,

I don't know what deep elements are. It looks like this is really more about getting the bounds of elements inside of a clipped container?

[Tim Horton]

(In reply to Simon Fraser (smfr) from comment #15)
> Comment on attachment 372509 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372509&action=review
> 
> > Source/WebCore/dom/Range.cpp:1824
> > +                    if (containingBlock.hasOverflowClip() && (style.overflowX() != Overflow::Visible || style.overflowY() != Overflow::Visible)) {
> 
> Maybe just containingBlock.hasOverflowClip()?

... really? I definitely did not get that from reading.

> > Source/WebCore/dom/Range.cpp:1839
> > +                    auto bounding = quad.boundingBox();
> 
> bounding is what springboks do

Is it.

> > Source/WebCore/dom/Range.h:119
> > +    enum class RectangleGenerationBehavior : uint8_t {
> 
> BoundingRectBehavior?

Sure.

> > Source/WebCore/dom/Range.h:121
> > +        IncludeRectsForDeepElements = 1 << 1,
> 
> I don't know what deep elements are. It looks like this is really more about
> getting the bounds of elements inside of a clipped container?

Previously, we would only include the bounds of each immediate child of the Range. Now, we recurse deeper into the tree instead. Doing this separately requires that we apply clipping to not regress many very simple cases.

[Tim Horton]

Created attachment 372545 [details]
Patch

[Tim Horton]

smfr suggests looking at what intersection observer uses for its clipped-bounds-of-element computation

[Tim Horton]

Created attachment 372597 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 372597 [details]
Patch

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

> Source/WebCore/dom/Range.cpp:1824
> +                    LayoutRect localBounds = renderer->borderBoundingBox();

auto

> Source/WebCore/dom/Range.cpp:1825
> +                    Optional<LayoutRect> rootClippedBounds = renderer->computeVisibleRectInContainer(localBounds, &renderer->view(),  {false, false, visibleRectOptions });

Weird spacing. Auto?

> Source/WebCore/dom/Range.cpp:1826
> +                    

No blank line

> Source/WebCore/dom/Range.cpp:1829
> +                    FloatRect snappedBounds = snapRectToDevicePixels(*rootClippedBounds, node->document().deviceScaleFactor());

auto

> Source/WebCore/dom/Range.h:121
> +        IncludeRectsForDeepElements = 1 << 1,

Would be better as UseVisibleBounds or something I think

> Source/WebCore/testing/Internals.cpp:5103
> +Internals::TextIndicatorData::TextIndicatorData(WebCore::TextIndicatorData data)

TextIndicatorData is big to pass by value.

> Source/WebCore/testing/Internals.cpp:5108
> +Internals::TextIndicatorData::~TextIndicatorData()

= default

> Source/WebCore/testing/Internals.h:833
> +    struct TextIndicatorData {

Name duplication with WebCore::TextIndicatorData is pretty confusing.

> Source/WebCore/testing/Internals.h:837
> +        TextIndicatorData(WebCore::TextIndicatorData);

halp

[Tim Horton]

Created attachment 372642 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 372642 [details]
Patch

Clearing flags on attachment: 372642

Committed r246695: <https://trac.webkit.org/changeset/246695>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.