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.

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

Fix #369 - Populate aria-owns attribute on iron-list and test #560

Merged
merged 3 commits into from
Oct 17, 2019
Merged

Conversation

hcarmona
Copy link
Contributor

@hcarmona hcarmona commented Oct 9, 2019

No description provided.

Copy link
Contributor

@kevinpschaaf kevinpschaaf left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! A couple of items of feedback.

iron-list.js Outdated
this._iterateItems(function(pidx, vidx) {
this.translate3d(0, y + 'px', 0, this._physicalItems[pidx]);
y += this._physicalSizes[pidx];
if (this._physicalItems[pidx].id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is in a hot path, can you capture const item = this._physicalItems[pidx] (used twice) and item.id (used twice) to cut down on the extra dereferences?

Let's also add this code in for the grid case above; I think it should apply the same, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, added constants for item and itemID.

I'm not sure a grid would handle this the same way since a grid has both rows and columns. The ownership of the columns should be on the row, and the rows would be owned by the grid. I haven't looked into this enough to be confident that the fix for a list would be the same as a fix for a grid.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not an ARIA expert, but even in grid mode the data source is still a one-dimensional array, so the row-item association is purely a presentation concept since it depends on screen width.

However, I just looked at the spec and apparently there are grid-specific ARIA attributes, and I'd have to learn more to know whether that applies for iron-list's type of grid or not (since we don't implement arrow-key 2D navigation, only tab). So for now I agree we'll just treat that as a separate issue and go ahead and get the list-only support merged.

@hcarmona hcarmona requested a review from kevinpschaaf October 11, 2019 21:46
@kevinpschaaf kevinpschaaf merged commit a45e328 into PolymerElements:master Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants