Skip to content

New rule: Content triggered on hover is hoverable (ep1s13) #1396

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

Open
wants to merge 73 commits into
base: develop
Choose a base branch
from

Conversation

carlosapaduarte
Copy link
Member

@carlosapaduarte carlosapaduarte commented Jul 17, 2020

Rule partially checking SC 1.4.13 (the hoverable part of SC 1.4.13)

Blocked: waiting for decision on how to handle states in rules

This will require a 2 weeks Final Call (new rule)


Pull Request Etiquette

How to Review And Approve

  • Go to the “Files changed” tab
  • Here you will have the option to leave comments on different lines.
  • Once the review is completed, find the “Review changes” button in the top right, select “Approve” (if you are really confident in the rule) or "Request changes" and click “Submit review”.
  • Make sure to also review the proposed Final Call period. In case of disagreement, the longer period wins.

@carlosapaduarte carlosapaduarte added Rule Use this label for a new rule that does not exist already reviewers wanted labels Jul 17, 2020
@carlosapaduarte carlosapaduarte self-assigned this Jul 17, 2020
@carlosapaduarte carlosapaduarte changed the title New rule: Content triggered on hover is hoverable (#ep1s13) New rule: Content triggered on hover is hoverable (ep1s13) Jul 21, 2020
@CLAassistant
Copy link

CLAassistant commented Jul 21, 2020

CLA assistant check
All committers have signed the CLA.

@carlosapaduarte
Copy link
Member Author

carlosapaduarte commented Dec 2, 2020

@WilcoFiers @kasperisager @ShadowBB
This one is ready for another round of reviews.
The applicability and expectations changed substantially from the previous version. Now it tries to identify content that became visible as a result of the hovering (new nodes plus nodes that change visibility) and that become invisible when the hovering ends. Expectations are that the content is adjacent to the hovered element (so that the user can move the pointer there without dismissing it) and that it does not go away for a testable amount of time (1 minute) while the pointer is at the center of the new content (once again, to ensure consistency while testing).
Please, have another go at it!

- DOM tree
---

[Bounding boxes][] A and B _overlap_ if at least one point inside box A has the same [left][left coordinate] and [top coordinates][top coordinate] of one point inside box B.
Copy link
Member

Choose a reason for hiding this comment

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

Points don't have a left / to pcoordinate. I would suggest you define this as any corner of element A is between the left and right, and top and bottom edges of element B, or visa versa.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. Thanks for the suggestion.

Comment on lines 13 to 14
- The [x coordinate][] is equal to the [left coordinate][] of the [bounding box][] plus half of the [width][] of the [bounding box][] (rounded down); and
- The [y coordinate][] is equal to the [top coordinate][] of the [bounding box][] plus half of the [height][] of the [bounding box][] (rounded down).
Copy link
Member

Choose a reason for hiding this comment

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

I think this is enough:

Suggested change
- The [x coordinate][] is equal to the [left coordinate][] of the [bounding box][] plus half of the [width][] of the [bounding box][] (rounded down); and
- The [y coordinate][] is equal to the [top coordinate][] of the [bounding box][] plus half of the [height][] of the [bounding box][] (rounded down).
- The [x coordinate][] is equal to the [left coordinate][] of the [bounding box][] plus half its [width][]; and
- The [y coordinate][] is equal to the [top coordinate][] of the [bounding box][] plus half its [height][].

- DOM tree
---

[Bounding boxes][] A and B are _adjacent_ if at least one point from box A is at the distance of 1 point from one point in box B.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[Bounding boxes][] A and B are _adjacent_ if at least one point from box A is at the distance of 1 point from one point in box B.
[Bounding boxes][] A and B are _adjacent_ if at least one point from box A is at the distance of 1 [CSS pixel[] from one point in box B.

- DOM tree
---

[Bounding boxes][] A and B are _adjacent_ if at least one point from box A is at the distance of 1 point from one point in box B.
Copy link
Member

Choose a reason for hiding this comment

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

Problem with this definition is that it also includes overlapping elements.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed


[Bounding boxes][] A and B are _adjacent_ if at least one point from box A is at the distance of 1 point from one point in box B.

The distance between two points is obtained by adding the horizontal distance between the two points with the vertical distance between the two points.
Copy link
Member

Choose a reason for hiding this comment

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

That doesn't sound right. So something with x:0,y0 has a distance of 2 to x:1,y:1? I don't think we should do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

What don't you think we should do? Consider (0,0) and (1,1) to not be adjacent or that the distance between them should not be 2? Or both?

In my opinion they are not adjacent, but its just an opinion and I don't feel too strong about that.

But I do feel the distance is 2 pixels (I have to move 1 pixel right and 1 pixel down to get from one point to the other). Pixels must be integer, therefore it cannot be the square root of 2.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think they are adjacent, and I don't think the distance is 2, it's 1.4px. The distance between two point is a straight line, what you're calculating is something else. Not sure what the right phrase for it is... or why using distance would be a problem here. Keep in mind that things don't have to be whole pixels distant from each other. Something can be 1/3rd of a pixel away from something else.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to come up with a distance metric that would always result in whole pixels. But, you're right, for distance we don't need to require that. I'll delete the definition of distance from this definition.

- DOM tree
---

The _content that becomes visible_ is the [root][] element, if one exists, of the [tree][] that contains all elements that, as result of an [event][] [firing][], meet any of the following:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not too sure what is going on here. How can a tree not have a root element? Do you maybe mean to find the closest common ancestor? How do I know that something is the "result of an event firing"? I don't think we should try to figure out if something is or isn't the result of an event. I think it's sufficient to determine that the content appears within half a second of firing the event, and that the content part of a region that updates without user input.

Copy link
Member Author

Choose a reason for hiding this comment

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

The "if one exists" was meant to apply to the tree, not the root element. My bad.

And I'll update to use the half a second instead of resulting from. That's definitely more testable!

Comment on lines 13 to 14
- the element has [visible text content][] and is added to the [web page][] where the [event][] was [fired][firing]; or
- the element [attributes][]' [values][] are modified in a way that cause some or all of its [text nodes][] to become [visible][].
Copy link
Member

Choose a reason for hiding this comment

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

Visible text content isn't bound to the closest ancestor. Any text nodes added to the page results in the HTML element having visible text content added.

How about something like:

Suggested change
- the element has [visible text content][] and is added to the [web page][] where the [event][] was [fired][firing]; or
- the element [attributes][]' [values][] are modified in a way that cause some or all of its [text nodes][] to become [visible][].
has visible text content that did not exist on the page prior to the event firing.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need to worry about the bounding because this tree only contains new elements (therefore elements that were there already are not considered) and elements that have had their attributes modified (therefore if their text nodes changed without a change in their attributes, they are not considered)

@carlosapaduarte
Copy link
Member Author

@WilcoFiers this is ready for another round


[Bounding boxes][] A and B are _adjacent_ if at least one point from box A is at the distance of 1 point from one point in box B.

The distance between two points is obtained by adding the horizontal distance between the two points with the vertical distance between the two points.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think they are adjacent, and I don't think the distance is 2, it's 1.4px. The distance between two point is a straight line, what you're calculating is something else. Not sure what the right phrase for it is... or why using distance would be a problem here. Keep in mind that things don't have to be whole pixels distant from each other. Something can be 1/3rd of a pixel away from something else.

Comment on lines 13 to 14
- The [x coordinate][] is equal to the [left coordinate][] of the [bounding box][] plus half its [width][] (rounded down); and
- The [y coordinate][] is equal to the [top coordinate][] of the [bounding box][] plus half its [height][] (rounded down).
Copy link
Member

Choose a reason for hiding this comment

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

Why do you want this rounded down? What's the use of that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was ensuring once again that coordinates were whole numbers. But they don't need to be (they're doubles).


The _content that becomes visible_ is the [root][] element of the [tree][], if a [tree][] exists, that contains all elements that, after half a second of an [event][] [firing][], meet any of the following:

- the element did not exist on the [web page][] prior to the [event][] [firing][] and has [visible text content][]; or
Copy link
Member

Choose a reason for hiding this comment

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

I think there is a fundamental problem in this. If I have an event rerenders the page, all elements are new, even if nothing changed. body.innerHTML = body.innerHTML will do that. Modern frameworks try to avoid unnecessary rerenders, but there is a lot of code on the web that rerenders components, irrespective of if something actually changed.

I think we need to define this in a way that works with rerenders. I took a stab at doing that just for text nodes, here's what I've come up with. That isn't enough, but I hope it gets helps move this forward.

A text node is considered to be _visible after an event_ if both of the following of the following are true:

- A tick before the event, there was no [visible][] text node with the same node value; and
- 500 milliseconds after the [event][] [firing][], there the text node is [visible][].

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that works because it requires that there are no text nodes with the same node value. If the new content includes a text node with the same value of an already present text node, then it will miss the new content.
I'll try to think of a solution for this...

Copy link
Member Author

Choose a reason for hiding this comment

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

Following your suggestion, check if you think that what I came up with works


The _content that becomes visible_ after an [event][] [firing][] is any element for which all of the following is true:

- 500 milliseconds after the [event][] [firing][] the element has [visible text content][]; and
Copy link
Member

Choose a reason for hiding this comment

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

We need to capture an assumption that content on hover will appear within 500ms.

The _content that becomes visible_ after an [event][] [firing][] is any element for which all of the following is true:

- 500 milliseconds after the [event][] [firing][] the element has [visible text content][]; and
- a tick before the [event][] [firing][] there was no element with the same [descendant text content][] that the element has after the [event][] [firing][]; and
Copy link
Member

Choose a reason for hiding this comment

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

Seems to me like this would apply to elements that update their content too. If I have a button that changes its text on hover, it 1. has visible content after the event. and 2. does not have the same descendant text content. Additionally, descendant text content does not take shadow DOM into account.

- DOM tree
---

[Bounding boxes][] A and B are _adjacent_ if they do not [overlap][] and at least one point from box A is at the Euclidean distance of 1 [CSS pixel][] from one point in box B.
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little nervous about this being exactly 1px with no margin of error. I think we should include anything less than 2, and greater than 0.

@carlosapaduarte carlosapaduarte added the Blocked Blocked by another PR/Issue label Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocked Blocked by another PR/Issue Rule Use this label for a new rule that does not exist already
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants