Bug 221328 - Make shouldComputeLogicalWidthFromAspectRatioAndInsets writing mode aware
Summary: Make shouldComputeLogicalWidthFromAspectRatioAndInsets writing mode aware
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks: 47738
  Show dependency treegraph
 
Reported: 2021-02-03 06:09 PST by Rob Buis
Modified: 2021-02-04 01:12 PST (History)
8 users (show)

See Also:


Attachments
Patch (3.97 KB, patch)
2021-02-03 06:11 PST, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2021-02-03 06:09:22 PST
Commit r271061 added support for aspect-ratio on positioned elements, taking into account
insets, but it failed to take writing-mode into account when checking insets. This change fixes
that by asking logical values.
Comment 1 Rob Buis 2021-02-03 06:11:49 PST
Created attachment 419128 [details]
Patch
Comment 2 Manuel Rego Casasnovas 2021-02-04 00:57:01 PST
Comment on attachment 419128 [details]
Patch

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

r=me, with a nit comment inline.

> Source/WebCore/rendering/RenderBox.cpp:5100
> +    if (style().width().isAuto() && style().height().isAuto() && !style().logicalTop().isAuto() && !style().logicalBottom().isAuto() && (style().logicalLeft().isAuto() || style().logicalRight().isAuto()))

What about logicalWidth() instead of width()? I guess it doesn't really matter as we have an and for width and height, but maybe it'd be worth changing for consistency dunno.
Comment 3 Rob Buis 2021-02-04 01:00:06 PST
Comment on attachment 419128 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:5100
>> +    if (style().width().isAuto() && style().height().isAuto() && !style().logicalTop().isAuto() && !style().logicalBottom().isAuto() && (style().logicalLeft().isAuto() || style().logicalRight().isAuto()))
> 
> What about logicalWidth() instead of width()? I guess it doesn't really matter as we have an and for width and height, but maybe it'd be worth changing for consistency dunno.

Interesting idea, but ultimately we need to check both dimensions and using the logical variants is a bit slower. So I'll keep as-is.
Comment 4 EWS 2021-02-04 01:11:34 PST
Committed r272363: <https://trac.webkit.org/changeset/272363>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419128 [details].
Comment 5 Radar WebKit Bug Importer 2021-02-04 01:12:14 PST
<rdar://problem/73970893>