I see that you just added the PR action: merge label, but the following checks are still failing:
     status "ci/circleci: integration_test" is failing
     status "google3" is pending
     missing required status "ci/circleci: publish_snapshot"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

Hi, the "ci/circleci: integration_test" is failing.

This seems to be the problem:

Commit 33877c6 uncompressed bundle exceeded expected size by >1% (expected: 177585, actual: 179726).

Is there anything I can do to fix this?

Edit: I tried to restart the CI job, but I don't seem to have permission.

@MartinMa the tests are failing because you added too much "weight" to the framework which has a negative impact on the payload size.

We'll need to discuss whether this is just something we have to swallow (and update the limit in the test), or if there are other measures that could allow these attributes while not increase the size. One to consider is allowing any "aria-" prefixed attribute without listing them specifically (this seems like a bad idea to me but @rjamet should weigh in on that option as well).

@MartinMa you brought up an interesting issue that has competing security, accessibility, and performance requirements. It's going to be interesting to see how we resolve it. 😄

No concern for security: I think the worse effect you'd get is something phishing-like, which is expected with a sanitizer, or confusing navigation/interactions (especially aria-activedescendant), which is also reasonable to expect from adversarial sanitized HTML.

@IgorMinar Interesting considerations.

A regex solution could be more lightweight. I think you'd probably have to change this line:

if (!VALID_ATTRS.hasOwnProperty(lower)) {

to this:

if (!(VALID_ATTRS.hasOwnProperty(lower) || ARIA_REGEX.test(lower))) {

while ARIA_REGEX could be one of the following

const ARIA_REGEX = /^aria-[a-z]*$/; // 1
const ARIA_REGEX = /^aria-[a-z]+$/; // 2
const ARIA_REGEX = /^aria-[a-z]{4,16}$/; // 3
const ARIA_REGEX = /^aria-(a(ctivedescendant|tomic|utocomplete)|busy|c(hecked|o(l(count|index|span)|ntrols)|urrent)|d(e(scribedby|tails)|isabled|ropeffect)|e(rrormessage|xpanded)|flowto|grabbed|h(aspopup|idden)|invalid|keyshortcuts|l(abel(|ledby)|evel|ive)|m(odal|ulti(line|selectable))|o(rientation|wns)|p(laceholder|osinset|ressed)|r(e(adonly|levant|quired)|oledescription|ow(count|index|span))|s(e(lected|tsize)|ort)|value(max|min|now|text))$/; // 4

I think something like 4 is a nightmare. 3 would cover the longest keyword aria-activedescendant, the short ones like aria-busy and everything in between.

Personally however, I am a fan of extending VALID_ATTRS as layed out in this PR and being strict about the attributes. But I could change the code to use a regex if desired?

If i understand correctly. Security is no concern with both approaches (strict list vs. relaxed regex)

Another question: Does the metric "uncompressed bundle size" count comments in?

@MartinMa ... This PR is really close to getting merged. Do you think you can figure out what is going wrong with the failing test in CI? It might be that it needs a rebase, I'm not sure, I didn't dig into it.

👍 Good luck

@benlesh I rebased the commit on top of master but the integration test is still failing because of:

Commit c10ca12 uncompressed bundle exceeded expected size by >1% (expected: 177585, actual: 179825).
If this is a desired change, please update the size limits in file '/home/circleci/ng/integration/../integration/_payload-limits.json'.

We were discussing if the payload limits in this tests should be adjusted accordingly ... (see the earlier comments).

Please tell me if there is anything else I can do.

Paging @IgorMinar for review... Should he update the payload size?

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

Presubmit

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}