HTML5 validation with table sticky header is misaligned over the toolbar

#800270: Anchor/focus hidden beneath toolbar/tableheader (CKEditor full screen menu) also handled keyboard navigation (TAB). Unfortunately no time to review.

#6: core-js-offset-mess-1870006-6.patch queued for re-testing.

1. +++ b/core/misc/offset/fragment.js
   @@ -0,0 +1,26 @@
   + * Offsets the window to allow fragments navigation with fixed positioning elements.
   

   80 col.

2. +++ b/core/misc/offset/fragment.js
   @@ -0,0 +1,26 @@
   +  "use strict";
   ...
   +  function adjustOffset () {
   

   Why this indentation? We don't do that elsewhere either.

   Also in other JS file.

3. +++ b/core/misc/offset/fragment.js
   @@ -0,0 +1,26 @@
   +  // Checks for a fragment on page load, outside Drupal behaviors.
   +  Drupal.behaviors.adjustOffset = { attach: adjustOffset };
   

   "outside" behaviors, yet you make it into a behavior?

4. +++ b/core/misc/offset/html5forms.js
   @@ -0,0 +1,13 @@
   + * Focus the HTML5 validation error in view.
   

   Cool :) But where can I see this in action?

5. +++ b/core/modules/system/system.module
   @@ -952,6 +952,10 @@ function system_library_info() {
          'core/misc/drupal.js' => array('group' => JS_LIBRARY, 'weight' => -18),
   +      'core/misc/debounce.js' => array('group' => JS_LIBRARY),
   +      'core/misc/displace.js' => array('group' => JS_LIBRARY),
   +      'core/misc/offset/fragment.js' => array('group' => JS_LIBRARY),
   +      'core/misc/offset/html5forms.js' => array('group' => JS_LIBRARY),
   

   Why? Because they can be considered fundamental?

   Shouldn't the html5forms thing be included with the drupal.form library, and drupal.displace only be loaded when the Toolbar is active, i.e. a dependency of Toolbar's library?

+++ b/core/modules/system/system.module
@@ -952,6 +952,10 @@ function system_library_info() {
       'core/misc/drupal.js' => array('group' => JS_LIBRARY, 'weight' => -18),
+      'core/misc/debounce.js' => array('group' => JS_LIBRARY),
+      'core/misc/displace.js' => array('group' => JS_LIBRARY),
+      'core/misc/offset/fragment.js' => array('group' => JS_LIBRARY),
+      'core/misc/offset/html5forms.js' => array('group' => JS_LIBRARY),

I agree, I'm having trouble parsing this. So it makes no sense for drupal.js to depend on the displace.js library, since displace.js itself relies on drupal.js. But since the displace library depends on the drupal library, we end up in something of a loop.

The only resolution I can come up with is explicitly declaring dependencies on the debounce, displace, and fragment libraries and letting them all depend on the drupal library, since it declares the Drupal object.

I'm tagging this for accessibility as it's required to get inline form errors as a fully fledged part of Core.

Beginning triage.

Verified that the issue still exists.
Updated issue summary.

I am adding credit to wolffereast for the triage work done.

Re rolled.
Not find all the issues.

.

1. diff --git a/core-js-offset-mess-1870006-10.patch b/core-js-offset-mess-1870006-10.patch
   

   Don't include the patch.

2. +++ b/core-js-offset-mess-1870006-10.patch
   @@ -0,0 +1,84 @@
   +diff --git a/core/misc/offset/fragment.js b/core/misc/offset/fragment.js
   

   Missing ES6 file.

3. +++ b/core-js-offset-mess-1870006-10.patch
   @@ -0,0 +1,84 @@
   +diff --git a/core/misc/offset/html5forms.js b/core/misc/offset/html5forms.js
   

   Missing ES6 file.

@pk188

We have changed the process for how JavaScript is built in D8. Have a look at [#2815083] to get a better understanding of what needs to happen.

Working on this now at the Haarlem sprint.

I applied the comments in #28. The patch in #26 was missing the JS includes from patch #10. I added the new JS files in the drupal.form library and added an extra dependency on drupal.displace.

assigning to myself for review at the Amsterdam a11y sprint

Reviewing the patch #31:

1. misc/offset/fragment.js is bundled with drupal.form. I think it should really be a dependency of the toolbar and table-header libraries, because these are the things we are trying to avoid overlapping with. Fragment IDs are found in content too. I've attached a bit of specimen HTML to simulate a long article with table-of-contents links and fragment IDs on headings (like a Wikipedia article). You can replicate the bug by following any of the TOC links. Use the attached fruits-toc-links-long-article.html.txt as the body content of a page.
2. There will be scenarios where core/drupal.tableheader and toolbar/toolbar could each be loaded without the other one, so fragment.js will need to be a core library which both can depend on.
3. Fragment IDs are not the only things which are accidentally obscured by the toolbar and tableheader. Form elements can be hidden as well. To experience this, you need a form which is much longer than the viewport.
   1. Create 50 nodes with devel_generate.module.
   2. visit admin/content.
   3. Hold down TAB, until your keyboard focus reaches the middle of the list, and the sticky tableheader engages.
   4. Now slowly press Shift-TAB to move focus backwards through the focusable elements (checkboxes, title links, and drop-buttons).
   5. Eventually, keyboard focus will be reach the rows hidden by the sticky table-header.
   6. Scroll with a mousewheel/trackpad, to reveal the focus checkbox under the table header.

   You will not reach this situation by following a fragment ID link, so a hashchange event handler isn't sufficient to fix this scenario. It's a realistic scenario for keyboard users who may need to traverse a form backwards to review their answers. It's mitigated by the fact that the up/down keys can scroll the viewport.

I've made some interdiffs to accompany the recent patches.

Maybe we could activate the fragment displace behaviour on focus as well as hashchange? fragment,js isn't the best name for in in that case.

the next step is to adjust the library declarations for toolbar module and core, with offset as a library on it;s own.

Here's a patch that introduces a new core library drupal.fragment.
We still need to thing about the third comment in #33.

Ah, my previous patch didn't include making the fragments library a dependency of toolbar/toolbar.

Regarding point 3: isn't this out-of-scope for this issue? I tested it, and it's a valid point. But the problem here with fragment navigation and form elements seems tackled.

Okay, we'll make #33.3 a follow-up issue. That's about when a form element comes into focus. Probably act on the focus event handler.

Code review of patch #37:

1. diff --git a/core/core.libraries.yml b/core/core.libraries.yml
   index 5da51e8ae5..ba8b8afb24 100644
   --- a/core/core.libraries.yml
   +++ b/core/core.libraries.yml
   @@ -215,13 +215,25 @@ drupal.form:
      version: VERSION
      js:
        misc/form.js: {}
   +    misc/offset/html5forms.js: {}
      dependencies:
        - core/jquery
        - core/drupal
        - core/drupal.debounce
   +    - core/drupal.displace
   +    - core/drupal.fragment
        - core/jquery.cookie
        - core/jquery.once
   

   core/drupal.displace is itself a dependency of core/drupal.fragment - do we actually have to make core/drupal.form depend on both of them? (I'm not sure what's standard.)

2. Likewise for core/drupal.tableheader dependencies.
3. Likewise for toolbar/toolbar dependencies.
4. core/misc/offset/html5forms.es6.js

   +    setTimeout(function () { window.scrollBy(0, -displace.offsets.top); }, 5);
   

   Need space between minus sign and displace.

5. core/misc/offset/html5forms.js

   +    setTimeout(function () { window.scrollBy(0, -displace.offsets.top); }, 5);
   

   Need space between minus sign and displace.

6. fragment.js uses window.scrollTo, but html5forms.js uses window.scrollBy. Is there a reason for using different methods?

Manual testing of patch #37:

1. Fragment identifier offset works, using my long article test content (attachment fruits-toc-links-long-article.html_.txt in #33). Following a link to a heading ID results in the heading being visible just below the toolbar. GOOD.
2. There's a quirk when the fragment identifier points to a form element. I tested this with inline form errors module, and @dmsmidt's errorstyle test module, which gives us a nice long form and IFE fragment links. When following an IFE fragment link to a text field, the <input> element comes into view, but the <label> is hidden by the toolbar. This is expected behaviour for what we currently have in <fragment.js, but it would be better if the <label> was brought into view too. Needs work.
3. A similar quirk with html5forms.js. I followed the steps to reproduce in the current issue summary (node form, jump to bottom, press save, html5 validation kicks in, title field is obscured). The title text input is correctly brought into view, but the label remains hidden under the toolbar. Needs work.
4. The placement of the html5 native error message differs between browsers. In Firefox 54/linux, html5forms.js brings the text input info view, but leaves the error message pointing to the original position under the toolbar, so the spatial relationship is wrong. In Chrome 59/linux, the error message is placed underneath the text input, and the spatial relationship is fine. I wonder if there's a timing issue here, with the order in which the browser places the message, and carries out the movement from the invalid event firing? I've no idea how to debug that. Screenshots attached for this quirk.

When bringing a form input into view, it would be desirable to bring the label into view too. Maybe we could add an extra step, to check if the element in question is a form input, and if so move the entire div.js-form-item container into view. We can identify them by matching the input ID to the container DIV classes, or to the label's for attribute. I don't know how robust this would be - it could break if a themer customizes the form element templates, say.

I didn't do any keyboard testing, just mouse clicking. Since this all fires on hashchange and invalid JS events, I'm not expecting any difference between keyboard and mouse use. Worth testing before committing though.

I'm really enjoying this issue. It's got some chewy edge cases, but it will make a huge difference when it's right.

Here are @andrewmacpherson's screenshots -- are these with the patch applied? If so I think we have more work to do especially for Firefox. :) Thanks @andrewmacpherson for the thorough review!

Firefox

Chrome (less bad)

Can we also add a screenshot for the unpatched state to the summary?

We agreed that this is a major accessibility bug. Thanks @wolffereast for helping triage!

1. +++ b/core/misc/offset/fragment.es6.js
   @@ -0,0 +1,26 @@
   +(function ($, Drupal, displace) {
   

   Arrow function and destructure these.

2. +++ b/core/misc/offset/fragment.es6.js
   @@ -0,0 +1,26 @@
   +  "use strict";
   

   Don't need use strict.

3. +++ b/core/misc/offset/fragment.es6.js
   @@ -0,0 +1,26 @@
   +  function adjustOffset () {
   

   Arrow function.

4. +++ b/core/misc/offset/fragment.es6.js
   @@ -0,0 +1,26 @@
   +        setTimeout(function () {
   

   Arrow function.

5. +++ b/core/misc/offset/html5forms.es6.js
   @@ -0,0 +1,13 @@
   +(function (displace) {
   

   Arrow function and destructure.

6. +++ b/core/misc/offset/html5forms.es6.js
   @@ -0,0 +1,13 @@
   +  "use strict";
   

   Don't need use strict.

7. +++ b/core/misc/offset/html5forms.es6.js
   @@ -0,0 +1,13 @@
   +  document.addEventListener('invalid', function (event) {
   

   Arrow function.

8. +++ b/core/misc/offset/html5forms.es6.js
   @@ -0,0 +1,13 @@
   +    setTimeout(function () { window.scrollBy(0, -displace.offsets.top); }, 5);
   

   Arrow function.

9. +++ b/core/misc/offset/html5forms.es6.js
   @@ -0,0 +1,13 @@
   +})(Drupal.displace);
   

   Pass `Drupal`.

- fixed minor issues mentioned in #42

After applying the patch, I didn't see any difference. It is exactly as on the screenshot

Note that the invalid scrollby in the html5forms component ALWAYS triggers. If you press 'save' while the title is visible, the window will still scroll, which is not the intended behaviour.

Please note that there is a discussion going to disable HTML5 form errors altogether here #1797438: HTML5 validation is preventing form submit and not fully accessible.
But probably only for Seven and Bartik themes, I expect a framework managers input there soon.

As a major bug report, this should still be eligible for 8.4.x
Requeing tests.

I have gone through the issue and did some research over it, which was already mentioned in the above comments.
I believe that it's more of a browser issue. I will continue my analysis & will post if I find something new.

Hi! Patch seems very promising, thank you!

After testing, it seems it's not perfect yet though : popup not properly placed, the form label is not shown, comment #48 note, etc.
I would like to talk about the "scroll-padding" CSS property that could lead to a simple CSS solution here (see https://css-tricks.com/almanac/properties/s/scroll-padding/).

@alexpott said in https://www.drupal.org/project/drupal/issues/2808699#comment-12099307 that a CSS solution could be preferable, and I totally agree. So, here is a patch with this approach instead of using Javascript.

Here is the result on my Drupal website with Firefox : https://res.cloudinary.com/renrhaf/image/upload/v1605212825/1870006-1_nq...
Note that I have increased by 30 pixels the amount of margin on my local installation for better result : https://res.cloudinary.com/renrhaf/image/upload/v1605212827/1870006-2_it...

The above patch works fine.
Before patch (Chrome)

After Patch (Chrome)

Before patch (Firefox)

After patch (Firefox)

Does having the CSS as fallback break the JS approach? Why not have both? I realize that is not adding much to the conversation.

Needed this patch with Drupal 8.9.x. Adding a re-rolled patch here for 8.9.x.

#65 works really nice. Thank you!

Just one thought: How about themes that add a sticky navbar? How can they make sure they won't run into the same problem as core's toolbar?

Apply patch and working fine sharing the screenshot of chrome and Firefox browser

Patch in #65 looks elegant and this is big improvement for editor experience. Manually tested with Claro and Seven themes.

+1 RTBC ! Thanks @nod_ for finalizing the patch :)

Attached after patch screenshot for Safari browser. seems fine to me.
RTBC +1

Random test fails for #65, Moving to RTBC again.

#3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest was committed. Ordered retest.

According to caniuse, scroll-padding is ignored for fragments in Safari, and doesn't work at all in IE11.

Are we happy with that compromise?

Assuming that Safari will get fixed at some point and that we don't need to provide identical experiences in IE.

Assuming nod_ wanted to re RTBC this

Restoring status after HEAD was broken.

Random unrelated failure, moving back to RTBC

Committed and pushed 9ec511c812 to 9.3.x and 8cb35ba074 to 9.2.x. Thanks!

I manually tested this in both chrome and firefox and it definitely improves things. There is another issue in firefox where the html5 error callout doesn't move when you scroll but that is a separate issue and it is not caused by this change. The firefox issue is https://bugzilla.mozilla.org/show_bug.cgi?id=643758

Leaving as 9.2.x because this is a JS change and being cautious.