Trying to address the fragment issue of points 4 and 6 from #1440628: [Meta] javascript toolbar/tableheader with url fragment mess.

4) tableheader + fragment: the browser will scroll so that the element with the targeted id shows up at the top of the screen. If you have elements set with position:fixed the fragment targets is hidden behind. #567754: Wrong anchor adjustment in tableheader.js
6) toolbar + fragment: this patch is supposed to fix it #546126: Toolbar interferes with anchor links but it does not work.

Patch needs work, properly define/inlude js libraries, clean up some code, testing but basis should be there.

Steps to reproduce

  1. Go to add new node form. Do not enter any information.
  2. Scroll down to the save button and ensure that the title field is above your viewport
  3. Click save, the title will be overlayed by the admin toolbar.

Comments

nod_’s picture

Status: Active » Needs review

NR to get some opinions.

Status: Needs review » Needs work

The last submitted patch, core-js-offset-mess.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review

Random faillure

nod_’s picture

FileSize
7.52 KB
PASSED: [[SimpleTest]]: [MySQL] 50,766 pass(es). View
nod_’s picture

Status: Needs review » Needs work

lots changed.

nod_’s picture

Status: Needs work » Needs review
FileSize
2.82 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-offset-mess-1870006-6.patch. Unable to apply patch. See the log in the details link for more information. View

Made a mess of everything, but it's upated to use the new Drupal.displace things and kind of works for fragments and html5 form errors. The sticky headers for tables are not taken into consideration but meh.

Putting NR for attention

casey’s picture

Priority: Normal » Major

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

mgifford’s picture

Status: Needs review » Needs work
Issue tags: +JavaScript clean-up, +Needs JS testing

The last submitted patch, core-js-offset-mess-1870006-6.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
2.77 KB
PASSED: [[SimpleTest]]: [MySQL] 59,006 pass(es). View
Wim Leers’s picture

Status: Needs review » Needs work
  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?

nod_’s picture

That's too cosmetic for the state the patch is in :þ

To test the HTML5 validation, just create a node, don't put a title, scroll down and try to save and publish.

jessebeach’s picture

+++ 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.

nod_’s picture

I put that in to not think about all this.

It's really the architecture of the JS code that needs work here before we get into anything else.

klonos’s picture

mgifford’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mgifford’s picture

Issue tags: +accessibility

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

dmsmidt’s picture

dmsmidt’s picture

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

wolffereast’s picture

Issue tags: +Baltimore2017

Beginning triage.

wolffereast’s picture

Version: 8.3.x-dev » 8.4.x-dev
Issue summary: View changes
Issue tags: -Needs issue summary update +Needs reroll, +Triaged for D8 major current state

Verified that the issue still exists.
Updated issue summary.

cilefen’s picture

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

pk188’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.49 KB

Re rolled.
Not find all the issues.

pk188’s picture

.

drpal’s picture

Status: Needs review » Needs work
  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.

dmsmidt’s picture

Issue tags: +sprint
BarisW’s picture

Assigned: Unassigned » BarisW

Working on this now at the Haarlem sprint.

BarisW’s picture

Assigned: BarisW » Unassigned
Status: Needs work » Needs review
FileSize
3.33 KB

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.

andrewmacpherson’s picture

Assigned: Unassigned » andrewmacpherson

assigning to myself for review at the Amsterdam a11y sprint

andrewmacpherson’s picture

Assigned: andrewmacpherson » Unassigned
Status: Needs review » Needs work
FileSize
27.54 KB
4.63 KB
6.32 KB

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.

andrewmacpherson’s picture

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.

andrewmacpherson’s picture

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

BarisW’s picture

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

BarisW’s picture

Status: Needs work » Needs review
FileSize
4.14 KB
294 bytes

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.

The last submitted patch, 36: core-js-offset-mess-1870006-36.patch, failed testing. View results

andrewmacpherson’s picture

Status: Needs review » Needs work
FileSize
94.63 KB
102.68 KB

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.