Remove Firefox workaround for details focus effect

?

The workaround I wrote is only needed for iOS.

And the case I was working around there is not a Firefox error or bug. It is a feature!
Firefox simply follows the operating system settings (that are ignored by Google Chrome for example).

https://bugzilla.mozilla.org/show_bug.cgi?id=756028#c1

I think we can remove it without worries.

Wow, so a "Firefox on iOS"-only work-around?!

I think we can remove it without worries.

🥳

Sorry, I was wrong. Not iOS. OS X. (The desktop.)

Updating tags to change the DrupalCon Amsterdam 2019 to 'Amsterdam2019' and temporarily removing 'novice' tag to reserve this for Contribution Day

Added the 'needs issue summary update' tag because there is little information about what the changes should be, missing information from the standard issue summary template.

In the attached patch the firefox workaround is being removed. The different behavior cannot be reproduced anymore via a comparison of latest Chrome (77.0.3865.120) to Firefox (70.0).

Re #11:

As I see, the patch removes all the focus styles.

We only want to remove the Firefox (and Safari) workaround which is here.

Removed the mentioned code in #12.

Re #13:

@Peter Majmesku, you missed compiling the es6. See https://www.drupal.org/node/2815083

I have built the js and can confirm there is now a small difference between Firefox on OSX and other browsers. It seems the focus effect is now applied for a short moment when the summary is clicked and then the outline disappears again.

@fcobbaert: Yep, the display in Firefox is different now. The outline disappears quickly on Firefox. In Chrome it does not. See Gif below (Chrome on the left, Firefox on the right).

It looks like the Firefox workaround is not really a workaround and should be kept. Shall we better close this issue?

It IS a workaround, but only for Firefox on OS X :)
(If you reach the focusable summary with keyboard, it works the way you expect it to.)

I've had a look at the patch from comment #15. Only the workaround has been removed.

When manually testing on Firefox (Mac OS) the focus is removed when the summary elements are clicked.

I've uploaded a video of a Chrome and Firefox comparison.

Changing to RTBC.

Re #20:

If you thoroughly check the desings you will notice that the focus ring belongs to the focus state, regardless of other states are present or not.

May proposal: if this is the only issue you noticed, lets open a separate issue for it and discuss it there.

This task is about the OS X Firefox workaround removal 🙂.

As discussed with @lauriii:

We also have to remove the flickering green focus of Firefox on OSX that can be seen in #17.
But only for Firefox, and only on OSX.

The behavior of other browsers and even Firefox on other operating systems must not change.

Tested with Firefox, Chrome and Safari, but only with OS X host, so further testing needed.

I tested #25 also with:

1. Windows 10:

   1. Firefox
   2. Chrome
   3. Microsoft Edge

2. Windows 8.1:

   1. Firefox
   2. Chrome
   3. Internet Explorer 11

3. Ubuntu 18.04:

   1. Firefox
   2. Chrome
4. Safari on iOS 13.0
5. Chrome for Android

It works as expected :) Could also anyone else confirm this?

Tested successfully in firefox on macos. I could reach the focusable summary with the keyboard. Also I could reach all advanced widgets with the tabulator key and enter key. So it LGTM.

@lauriii just asked for an accessibility review on Slack. The issue summary is vague, and doesn't describe the behaviour you're concerned with. So I'll take some time to read all the comments.

I chatted with @lauriii on Slack about this, but I'm still confused about exactly what the problem and resolution are.

This is tricky for me to review, because I don't have a working mac just now. (I normally do, but it stopped booting after the 10.15.1 upgrade.)

This issue needs more detail, please:

• An explanation of what the problem is, and the workaround.
• Steps to reproduce
• Whether any macOS system settings make a difference. Comment #4 mentions an operating system preference, but doesn't say which one.
• An explanation of what the animated GIFs are showing. I can't tell what user actions are taking place (keypresses, mouse clicks), but I can see the pointer move. It's also hard to follow because it loops, and I don't know where the start is, and the actions are fast.

Are any other elements affected? The bugzilla report linked in #4 mentions checkboxes and radios.

How does Safari behave? The issue summary talks about inconsistency between Firefox and Chrome.

What is actually proposed here? #28 says it works as expected, but I can't tell what behaviour you are looking for.

Re-rolled #25.

Uploading videos taken about W3Schools demo.

Pointer clicks are visible, every other actions are [tab/space/enter] key hits.

What you will notice:
Firefox on OS X shows summary's focus only if the summary was reached by keyboard navigation, and if it was expanded/collapsed by key events (hitting Space or Enter).
Even if the summary had the focus indication before, if the user clicks on it, the focus indication will be removed.

Every other browser, and even Firefox on other platforms (Windows, Ubuntu), this focus indication is visible for pointer events as well.

Attached two more videos.

1. Firefox-OSX--with-workaround.mp4 shows how details summary focus works right now. This is exactly the same behavior that users (will) experience with other browsers.
2. Details-focus--Firefox-Ubuntu.mp4 shows how Firefox on OS X manages details summary focus without the workaround (with patch #35).

   Other browsers and Firefox on other operating systems will keep having the same behavior that is illustrated by the previous point.

Re-rolled #35 and updated IS (#32).

Re #32:
I added some videos in #36 and in #38 that should clarify what this issue is about.

Re #33:

1. No other elements were affected by the workaround that we plan to remove here.
2. Safari does the same as any other browsers.
3. Answered in the IS Problem/Motivation and in User interface changes

Re-rolled #35

This is a lie.

#42 Thanks for the summary of our test session @lauriii, both here and the new upstream Mozilla issue.

Comment #4 (and the Mozilla issue li) suggests that it's following an established macOS behaviour (and/or a setting), but I've not been able to verify what policy it's following (or which setting). The new issue you filed includes the important observation that Firefox is behaving differently to Safari, which casts doubt on the "following mac convention" claim.

Supporting user-agent and/or OS behaviours is generally a good idea, but in this case I think it's a poor experience for people who make mixed use of keyboard and mouse. So I think we should continue to normalize this across browsers. If we find a clearer explanation (or the name of a user setting) for Firefox's behaviour, that would be a different matter.

I suggest we keep the Claro workaround for now, and look at the best way to make it generic and robust.

I'm a little concerned that the current code might lead to focus events being fired twice in some cases. Focus usually comes after mousedown, but before click, so we could put in a check for the activeElement to avoid firing focus twice?

This is a reroll of #41, which I manually confirmed still works, and can be seen in the attached gif.

#48 Patch Failed

#52 Please review the patch

I have added with a workaround and without workaround screen recording after patch #52.

I don't find any difference. Please let me know if I am doing something wrong.

I have tested this issue on Firefox.
The issue is not reproducible on the 9.3.x-dev version.
I have attached the video before and after applying the patch #52.

I don't find any difference. Please let me know if anything missed from my side.
Need +1 RTBC

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I created issue fork branch and rerolled patch #52.

Change from patch #52:

1. Remove jQuery calling in /claro/js/details.js
2. Remove workaround for weird flash.

   I cannot reproduce comment #17 on 11.x branch and Firefox 146

3. Remove :active pseudo class from selector for vertical tab.

Please review.

This one needs a rebase and manual testing. May be a good contribution item for fldc26