[PEPC] Add tests that check container CSS affects validation [chromium/src : main]

0 views
Skip to first unread message

Andy Paicu (Gerrit)

unread,
Apr 12, 2024, 9:09:20 AMApr 12
to Thomas Nguyen, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Thomas Nguyen

Andy Paicu added 1 comment

Patchset-level comments
File-level comment, Patchset 1 (Latest):
Andy Paicu . resolved

Hi Thomas, PTAL.

Open in Gerrit

Related details

Attention is currently required from:
  • Thomas Nguyen
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: If0a6e1a7a3ba88de6ec43aea6e20944b81ebdd12
Gerrit-Change-Number: 5447702
Gerrit-PatchSet: 1
Gerrit-Owner: Andy Paicu <andy...@chromium.org>
Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
Gerrit-Reviewer: Thomas Nguyen <tun...@chromium.org>
Gerrit-Attention: Thomas Nguyen <tun...@chromium.org>
Gerrit-Comment-Date: Fri, 12 Apr 2024 13:09:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Thomas Nguyen (Gerrit)

unread,
Apr 15, 2024, 4:54:41 AMApr 15
to Andy Paicu, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
Attention needed from Andy Paicu

Thomas Nguyen added 1 comment

File third_party/blink/renderer/core/html/html_permission_element_test.cc
Line 984, Patchset 1 (Latest): "rotate(0.1turn)");
Thomas Nguyen . unresolved

I need to understand why this transform will disable the PEPC. This will rotate the container (+ PEPC inside). Following the visibility logic, the PEPC is still visible and should be clickable.

Open in Gerrit

Related details

Attention is currently required from:
  • Andy Paicu
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: If0a6e1a7a3ba88de6ec43aea6e20944b81ebdd12
    Gerrit-Change-Number: 5447702
    Gerrit-PatchSet: 1
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Thomas Nguyen <tun...@chromium.org>
    Gerrit-Attention: Andy Paicu <andy...@chromium.org>
    Gerrit-Comment-Date: Mon, 15 Apr 2024 08:54:34 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Andy Paicu (Gerrit)

    unread,
    Apr 15, 2024, 7:06:51 AMApr 15
    to Thomas Nguyen, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
    Attention needed from Thomas Nguyen

    Andy Paicu added 1 comment

    File third_party/blink/renderer/core/html/html_permission_element_test.cc
    Line 984, Patchset 1 (Latest): "rotate(0.1turn)");
    Thomas Nguyen . unresolved

    I need to understand why this transform will disable the PEPC. This will rotate the container (+ PEPC inside). Following the visibility logic, the PEPC is still visible and should be clickable.

    Andy Paicu

    The IntersectionObserver only allows non-distorting visual effects such as translations and up-scaling [1]. Rotations are not allowed, probably due to how complicated it would be to compute this. I don't think this should be a problem for us.


    [1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/layout_object.cc;drc=a002cb035f709dcb195fd5c8a1bb2f7b2260c613;l=2073

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Thomas Nguyen
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: If0a6e1a7a3ba88de6ec43aea6e20944b81ebdd12
    Gerrit-Change-Number: 5447702
    Gerrit-PatchSet: 1
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Thomas Nguyen <tun...@chromium.org>
    Gerrit-Attention: Thomas Nguyen <tun...@chromium.org>
    Gerrit-Comment-Date: Mon, 15 Apr 2024 11:06:43 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Thomas Nguyen <tun...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Thomas Nguyen (Gerrit)

    unread,
    Apr 15, 2024, 7:44:58 AMApr 15
    to Andy Paicu, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
    Attention needed from Andy Paicu

    Thomas Nguyen voted and added 1 comment

    Votes added by Thomas Nguyen

    Code-Review+1

    1 comment

    File third_party/blink/renderer/core/html/html_permission_element_test.cc
    Line 984, Patchset 1 (Latest): "rotate(0.1turn)");
    Thomas Nguyen . unresolved

    I need to understand why this transform will disable the PEPC. This will rotate the container (+ PEPC inside). Following the visibility logic, the PEPC is still visible and should be clickable.

    Andy Paicu

    The IntersectionObserver only allows non-distorting visual effects such as translations and up-scaling [1]. Rotations are not allowed, probably due to how complicated it would be to compute this. I don't think this should be a problem for us.


    [1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/layout_object.cc;drc=a002cb035f709dcb195fd5c8a1bb2f7b2260c613;l=2073

    Thomas Nguyen

    It seems to be disabled permanently. In this case, users and devs will have no idea why it fails and not be directed to the root cause and how to fix it.

    I am not opposing, but a task for us is show the useful/correct information in this case (eg: a log like " the element is not visible" is not relevant)

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Andy Paicu
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: If0a6e1a7a3ba88de6ec43aea6e20944b81ebdd12
    Gerrit-Change-Number: 5447702
    Gerrit-PatchSet: 1
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Thomas Nguyen <tun...@chromium.org>
    Gerrit-Attention: Andy Paicu <andy...@chromium.org>
    Gerrit-Comment-Date: Mon, 15 Apr 2024 11:44:44 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    Comment-In-Reply-To: Andy Paicu <andy...@chromium.org>
    Comment-In-Reply-To: Thomas Nguyen <tun...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Andy Paicu (Gerrit)

    unread,
    Apr 15, 2024, 7:49:22 AMApr 15
    to Thomas Nguyen, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
    Attention needed from Thomas Nguyen

    Andy Paicu added 1 comment

    File third_party/blink/renderer/core/html/html_permission_element_test.cc
    Line 984, Patchset 1: "rotate(0.1turn)");
    Thomas Nguyen . unresolved

    I need to understand why this transform will disable the PEPC. This will rotate the container (+ PEPC inside). Following the visibility logic, the PEPC is still visible and should be clickable.

    Andy Paicu

    The IntersectionObserver only allows non-distorting visual effects such as translations and up-scaling [1]. Rotations are not allowed, probably due to how complicated it would be to compute this. I don't think this should be a problem for us.


    [1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/layout_object.cc;drc=a002cb035f709dcb195fd5c8a1bb2f7b2260c613;l=2073

    Thomas Nguyen

    It seems to be disabled permanently. In this case, users and devs will have no idea why it fails and not be directed to the root cause and how to fix it.

    I am not opposing, but a task for us is show the useful/correct information in this case (eg: a log like " the element is not visible" is not relevant)

    Andy Paicu

    Should we expand the console message then to include also transforms?

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Thomas Nguyen
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: If0a6e1a7a3ba88de6ec43aea6e20944b81ebdd12
    Gerrit-Change-Number: 5447702
    Gerrit-PatchSet: 2
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Thomas Nguyen <tun...@chromium.org>
    Gerrit-Attention: Thomas Nguyen <tun...@chromium.org>
    Gerrit-Comment-Date: Mon, 15 Apr 2024 11:49:11 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Thomas Nguyen (Gerrit)

    unread,
    Apr 15, 2024, 8:26:05 AMApr 15
    to Andy Paicu, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
    Attention needed from Andy Paicu

    Thomas Nguyen voted and added 1 comment

    Votes added by Thomas Nguyen

    Code-Review+1

    1 comment

    File third_party/blink/renderer/core/html/html_permission_element_test.cc
    Line 984, Patchset 1: "rotate(0.1turn)");
    Thomas Nguyen . unresolved

    I need to understand why this transform will disable the PEPC. This will rotate the container (+ PEPC inside). Following the visibility logic, the PEPC is still visible and should be clickable.

    Andy Paicu

    The IntersectionObserver only allows non-distorting visual effects such as translations and up-scaling [1]. Rotations are not allowed, probably due to how complicated it would be to compute this. I don't think this should be a problem for us.


    [1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/layout_object.cc;drc=a002cb035f709dcb195fd5c8a1bb2f7b2260c613;l=2073

    Thomas Nguyen

    It seems to be disabled permanently. In this case, users and devs will have no idea why it fails and not be directed to the root cause and how to fix it.

    I am not opposing, but a task for us is show the useful/correct information in this case (eg: a log like " the element is not visible" is not relevant)

    Andy Paicu

    Should we expand the console message then to include also transforms?

    Thomas Nguyen

    Yes we should, the idea is, we should show a log when the PEPC is disabled *permanently*. We only had logs on several static checks like security context: permission policy and fenced frame, currently the visibility check is from IntersectionObserver -> we are missing it (or might be misaligned)

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Andy Paicu
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: If0a6e1a7a3ba88de6ec43aea6e20944b81ebdd12
    Gerrit-Change-Number: 5447702
    Gerrit-PatchSet: 2
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Thomas Nguyen <tun...@chromium.org>
    Gerrit-Attention: Andy Paicu <andy...@chromium.org>
    Gerrit-Comment-Date: Mon, 15 Apr 2024 12:25:53 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Andy Paicu (Gerrit)

    unread,
    Apr 15, 2024, 8:54:13 AMApr 15
    to Mason Freed, Thomas Nguyen, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
    Attention needed from Mason Freed

    Andy Paicu added 2 comments

    Patchset-level comments
    File-level comment, Patchset 2 (Latest):
    Andy Paicu . resolved

    Hi Mason, PTAL.

    File third_party/blink/renderer/core/html/html_permission_element_test.cc
    Line 984, Patchset 1: "rotate(0.1turn)");
    Thomas Nguyen . resolved

    I need to understand why this transform will disable the PEPC. This will rotate the container (+ PEPC inside). Following the visibility logic, the PEPC is still visible and should be clickable.

    Andy Paicu

    The IntersectionObserver only allows non-distorting visual effects such as translations and up-scaling [1]. Rotations are not allowed, probably due to how complicated it would be to compute this. I don't think this should be a problem for us.


    [1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/layout_object.cc;drc=a002cb035f709dcb195fd5c8a1bb2f7b2260c613;l=2073

    Thomas Nguyen

    It seems to be disabled permanently. In this case, users and devs will have no idea why it fails and not be directed to the root cause and how to fix it.

    I am not opposing, but a task for us is show the useful/correct information in this case (eg: a log like " the element is not visible" is not relevant)

    Andy Paicu

    Should we expand the console message then to include also transforms?

    Thomas Nguyen

    Yes we should, the idea is, we should show a log when the PEPC is disabled *permanently*. We only had logs on several static checks like security context: permission policy and fenced frame, currently the visibility check is from IntersectionObserver -> we are missing it (or might be misaligned)

    Andy Paicu

    Since we have a task to add logs for developers that we might have missed. Let's leave it as part of that.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Mason Freed
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: If0a6e1a7a3ba88de6ec43aea6e20944b81ebdd12
    Gerrit-Change-Number: 5447702
    Gerrit-PatchSet: 2
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
    Gerrit-Reviewer: Thomas Nguyen <tun...@chromium.org>
    Gerrit-Attention: Mason Freed <mas...@chromium.org>
    Gerrit-Comment-Date: Mon, 15 Apr 2024 12:54:00 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    open
    diffy

    Mason Freed (Gerrit)

    unread,
    Apr 17, 2024, 9:01:47 PMApr 17
    to Andy Paicu, Thomas Nguyen, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org
    Attention needed from Andy Paicu

    Mason Freed voted Code-Review+1

    Code-Review+1
    Open in Gerrit

    Related details

    Attention is currently required from:
    • Andy Paicu
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: If0a6e1a7a3ba88de6ec43aea6e20944b81ebdd12
    Gerrit-Change-Number: 5447702
    Gerrit-PatchSet: 2
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
    Gerrit-Reviewer: Thomas Nguyen <tun...@chromium.org>
    Gerrit-Attention: Andy Paicu <andy...@chromium.org>
    Gerrit-Comment-Date: Thu, 18 Apr 2024 01:01:38 +0000
    Gerrit-HasComments: No
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    open
    diffy

    Andy Paicu (Gerrit)

    unread,
    Apr 18, 2024, 3:48:23 AMApr 18
    to Mason Freed, Thomas Nguyen, Chromium LUCI CQ, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org

    Andy Paicu voted and added 1 comment

    Votes added by Andy Paicu

    Commit-Queue+2

    1 comment

    Patchset-level comments
    File-level comment, Patchset 3 (Latest):
    Andy Paicu . resolved

    Thank you both

    Open in Gerrit

    Related details

    Attention set is empty
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: If0a6e1a7a3ba88de6ec43aea6e20944b81ebdd12
    Gerrit-Change-Number: 5447702
    Gerrit-PatchSet: 3
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
    Gerrit-Reviewer: Thomas Nguyen <tun...@chromium.org>
    Gerrit-Comment-Date: Thu, 18 Apr 2024 07:48:10 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    open
    diffy

    Chromium LUCI CQ (Gerrit)

    unread,
    Apr 18, 2024, 4:34:15 AMApr 18
    to Andy Paicu, Mason Freed, Thomas Nguyen, chromium...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org

    Chromium LUCI CQ submitted the change with unreviewed changes

    Unreviewed changes

    2 is the latest approved patch-set.
    No files were changed between the latest approved patch-set and the submitted one.

    Change information

    Commit message:
    [PEPC] Add tests that check container CSS affects validation

    There are various properties that can be applied to parent elements
    that would affect the validity of the PEPC element. These are covered
    by the IntersectionObserver visibility logic. This CL only adds tests
    to verify the behavior.

    Also a small bug fix to the "disable" logic where an indefinite disable
    would not override a temporary disable. The scenario is verified in
    *.IntersectionChangedDisableEnableDisable
    Fixed: 335250088
    Bug: 1462930
    Change-Id: If0a6e1a7a3ba88de6ec43aea6e20944b81ebdd12
    Commit-Queue: Andy Paicu <andy...@chromium.org>
    Reviewed-by: Thomas Nguyen <tun...@chromium.org>
    Reviewed-by: Mason Freed <mas...@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#1289196}
    Files:
    • M third_party/blink/renderer/core/html/html_permission_element.cc
    • M third_party/blink/renderer/core/html/html_permission_element_test.cc
    Change size: M
    Delta: 2 files changed, 82 insertions(+), 1 deletion(-)
    Branch: refs/heads/main
    Submit Requirements:
    • requirement satisfiedCode-Review: +1 by Thomas Nguyen, +1 by Mason Freed
    Open in Gerrit
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: merged
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: If0a6e1a7a3ba88de6ec43aea6e20944b81ebdd12
    Gerrit-Change-Number: 5447702
    Gerrit-PatchSet: 4
    Gerrit-Owner: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Andy Paicu <andy...@chromium.org>
    Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
    Gerrit-Reviewer: Mason Freed <mas...@chromium.org>
    Gerrit-Reviewer: Thomas Nguyen <tun...@chromium.org>
    open
    diffy
    satisfied_requirement
    Reply all
    Reply to author
    Forward
    0 new messages