onSizeChanged reports size without padding [177562900]

Fixed

Bug

Status Update

No update yet.

Description

vs...@gmail.com

created issue #1

Jan 14, 2021 06:35PM

Jetpack Compose release version: 1.0.0-alpha10

Steps to Reproduce:

@Preview
@Composable
fun Test() {
    val density = AmbientDensity.current
    Box(
        modifier = Modifier
            .onSizeChanged { println(it) }
            .padding(with(density) { 50.toDp() })
            .size(with(density) { 50.toDp() })
            .background(Color.Red)
    )
}

Expected printout: 150 x 150
Actual printout: 50 x 50

On the other hand if I replace onSizeChanged with layout it prints expected result.

modifier = Modifier
    .layout { measurable, constraints ->
        val placeable = measurable.measure(constraints)
        println("${placeable.width} x ${placeable.height}")
        layout(placeable.width, placeable.height) {
            placeable.place(0, 0)
        }
    }
    .padding(with(density) { 50.toDp() })
    .size(with(density) { 50.toDp() })
    .background(Color.Red)

Comments

po...@google.com <po...@google.com> Jan 14, 2021 06:39PM

Assigned to an...@google.com.

an...@google.com <an...@google.com> #2Jan 14, 2021 07:10PM

Reassigned to mo...@google.com.

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

mo...@google.com <mo...@google.com> #3Jan 14, 2021 08:33PM

This is an interesting question of what the developer wants. We'll have to discuss this as it is working as designed, but may not be working as the developer wants!

vs...@gmail.com <vs...@gmail.com> #4Jan 14, 2021 09:13PM

Personally it just came unexpected for me because I didn't catch a concept of inner content for composable. I always thought about padding as a part of any content where it happens to be attached to. Maybe documentation should be a bit more specific about that.

Regarding the usefulness of such modifier that will count the padding in, I can say that I immediately added the helper extension in my project which I called onSizeChangedPaddingIncluded :) It is useful part for my implementation of drag-n-drop reordering machinery for LazyColumn.

ae...@google.com <ae...@google.com> #5Jan 14, 2021 09:58PM

I see. We just ran into this in a foundation component CL as well: https://android-review.googlesource.com/c/platform/frameworks/support/+/1537302/5..6/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text/CoreTextField.kt#b445 . We are forced to introduce an extra Box because of the current behavior.

ap...@google.com <ap...@google.com> #6Jan 19, 2021 08:27PM

Marked as fixed.

Project: platform/frameworks/support
Branch: androidx-main

commit 940f677411cf9f83c5bfceb6f6db588de846e48b
Author: George Mount <mount@google.com>
Date: Fri Jan 15 20:59:22 2021

OnSizeChanged now reports the size within the modifier chain.

Relnote: "OnSizeChanged was reporting the size of the layout's
contents. It now reports the size at its position within the
modifier chain."

Fixes: 177562900
Test: new test
Change-Id: I36b78f445cf90b10f3df4ea6251bc19392bb7c11

M compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/layout/OnSizeChangedTest.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/layout/OnRemeasuredModifier.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/node/LayoutNode.kt
A compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/node/RemeasureModifierWrapper.kt

https://android-review.googlesource.com/1552681

Issue 177562900