Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

♻️ Remove role button from buttons #32252

Merged
merged 5 commits into from
Jan 27, 2021
Merged

♻️ Remove role button from buttons #32252

merged 5 commits into from
Jan 27, 2021

Conversation

kristoferbaxter
Copy link
Contributor

Addresses #31342.

In particular to look at @gmajoulet are the changes to Web Stories components.

@amp-owners-bot
Copy link

amp-owners-bot bot commented Jan 27, 2021 •
edited

Hey @gmajoulet! These files were changed:

extensions/amp-story-360/0.1/amp-story-360.js
extensions/amp-story/1.0/amp-story-embedded-component.js
extensions/amp-story/1.0/amp-story-page.js

Hey @processprocess, @t0mg! These files were changed:

extensions/amp-story-360/0.1/amp-story-360.js

Hey @newmuis, @Enriqe! These files were changed:

extensions/amp-story/1.0/amp-story-embedded-component.js
extensions/amp-story/1.0/amp-story-page.js

caroqliu
caroqliu approved these changes Jan 27, 2021
View reviewed changes
examples/adsense.amp.html
@@ -58,7 +58,7 @@
id="amp-user-notification">
This site uses cookies to personalize content.
<a href="#learn-more">Learn more.</a>
<button on="tap:amp-user-notification.dismiss" role="button" tabindex="0">I accept</button>
<button on="tap:amp-user-notification.dismiss" tabindex="0">I accept</button>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Buttons are also focusable, can we remove any tabindex="0"? (other #s are meaningful and can be left as is for providing focus order though)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do this in a followup PR. This one touches a lot of files already.

examples/amp-consent/cmp-vendors.amp.html Outdated Show resolved Hide resolved
Enriqe
Enriqe approved these changes Jan 27, 2021
View reviewed changes
Copy link
Contributor

@Enriqe Enriqe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

amp-story/* approval

@kristoferbaxter kristoferbaxter merged commit 2f47360 into ampproject:master Jan 27, 2021
@kristoferbaxter kristoferbaxter deleted the role-button-on-button branch January 27, 2021 20:41
@kristoferbaxter kristoferbaxter mentioned this pull request Jan 27, 2021
Closed
PetrBlaha pushed a commit to PetrBlaha/amphtml that referenced this pull request Jan 28, 2021
@kristoferbaxter @caroqliu @PetrBlaha
♻️ Remove role button from buttons (ampproject#32252)
98acab9 
* Remove role button from buttons

* Update examples/amp-consent/cmp-vendors.amp.html

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

* Missed one in amp-story-page

* two more files were missed

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>
@gmajoulet
Copy link
Contributor

This PR needs to be rollbacked, or a fix needs to be sent in the next hour.

Stories use the role="button" to know not to navigate to the next page on click. I believe clicking the "Play video" fallback button, or "Enable 360" button will now incorrectly navigate to the next story page.

cc @Enriqe @kristoferbaxter

@kristoferbaxter
Copy link
Contributor Author

Feel free to roll this PR back, but I'd also ask to do the following:

  1. Stop relying on role=button for navigation canceling, enhancing to more standard DOM semantics.
  2. Add tests so those less familiar with the amp-story code base don't make this same change.

@Enriqe Enriqe mentioned this pull request Jan 28, 2021
Merged
@Enriqe
Copy link
Contributor

Enriqe commented Jan 28, 2021

PR for story revert: #32283

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Enriqe Enriqe Enriqe approved these changes

@caroqliu caroqliu caroqliu approved these changes

@gmajoulet gmajoulet Awaiting requested review from gmajoulet

@micajuine-ho micajuine-ho Awaiting requested review from micajuine-ho

@processprocess processprocess Awaiting requested review from processprocess

Assignees
No one assigned
Labels
PR use: In Beta / Experimental PR use: In Stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kristoferbaxter @gmajoulet @Enriqe @caroqliu @ampprojectbot