Fix internal/local navigation links scrolling #5381

mkhatib · 2016-10-03T20:24:43Z

Updating tests. @dvoytenko let me know if this looks good.

Demo here try it on iOS simulator - second tab, open sidebar and click section 1 and 2 links to see it in effect, close the side bar notice everything works as expected.

mkhatib · 2016-10-03T20:48:58Z

Demo here try it on iOS simulator - second tab, open sidebar and click section 1 and 2 links to see it in effect, close the side bar notice everything works as expected.

jridgewell · 2016-10-03T20:52:27Z

src/document-click.js

+  // the scrollbar jumping the user back to the top for failing to calculate
+  // the new jumped offset.
+  // See https://github.com/ampproject/amphtml/issues/5334 for more details.
+  let oldAttribute = {};
  if (hash) {
    elem = ampdoc.getRootNode().getElementById(hash);
    if (!elem) {


Please swap this conditional.

Dropped. Using || instead.

jridgewell · 2016-10-03T20:53:07Z

src/document-click.js

-  win.location.replace(`#${hash}`);
+  // for more details. Do this only if fragment has changed.
+  if (tgtLoc.hash != curLoc.hash) {
+    win.location.replace(`#${hash}`);


Do we want to preserve the share tracking hash?

Not sure. @dvoytenko do you know how do we handle this here?

I don't think it's a good idea to have both document tag and share-tracking hash together in the url hash. Maybe we could only have document tag in this case. So I think this is good for now.

Yeah, don't preserve. It's up to the publisher on whether they use hash-based links.

jridgewell · 2016-10-03T20:54:24Z

src/document-click.js

+      }
+    } else {
+      oldAttribute = {key: 'id', value: elem.id};
+      elem.removeAttribute('id');


We need to ensure there's not a [name=${hash}] element, too. And there could be multiple of both.

Good points. Will fix.

mkhatib · 2016-10-03T20:55:47Z

src/document-click.js

  } else {
    dev().warn('documentElement',
        `failed to find element with id=${hash} or a[name=${hash}]`);
  }

+  // Has fragment really changed?
+  if (tgtLoc.hash != curLoc.hash) {


This is == (caught by a test). Fixing.

dvoytenko · 2016-10-03T20:56:00Z

src/document-click.js

+        oldAttribute = {key: 'name', value: elem.name};
+        elem.removeAttribute('name');
+      }
+    } else {


Could you please revert if/else here to remove double negative?

dvoytenko · 2016-10-03T20:58:01Z

src/document-click.js

  } else {
    dev().warn('documentElement',
        `failed to find element with id=${hash} or a[name=${hash}]`);
  }

+  // Has fragment really changed?
+  if (tgtLoc.hash != curLoc.hash) {


It should be == here I believe. Also, please remove return and wrap the history.push inside the if.

Yes, caught by a test. Will fix

mkhatib · 2016-10-03T21:51:20Z

PTAL 👀

jridgewell · 2016-10-03T21:54:36Z

src/document-click.js

+function removeAttrsWithMatchingHash_(ampdoc, hash) {
+  const restoreElementsAttrs = [];
+  const targetElements = ampdoc.getRootNode().querySelectorAll(
+      `#${hash},a[name=${hash}]`) || [];


The hash should be escaped and quoted for the name part.

By the hash you mean the literal # sign or the variable ${hash}? If the earlier, why do we need to escape it?

If the latter, what values do we expect to break this? And what does escape means here?

hash = something] will break this. @dvoytenko recently did something with CSS escaping.

Used dom.escapeCssSelectorIdent to escape it. @dvoytenko is this accurate? Or if we should just use the css-escape.cssEscape instead?

Yes, dom.escapeCssSelectorIdent would do the trick.

jridgewell · 2016-10-03T21:56:01Z

src/document-click.js

+      `#${hash},a[name=${hash}]`) || [];
+  for (let i = 0; i < targetElements.length; i++) {
+    const element = targetElements[i];
+    const attributes = {};


We can simplify to:

attributes = { name: element.name === hash, id: element.id === name, };

Then I'd have to check the value when looping over attrs. I also still need the if else to remove attributes when matching.

Ohh, I see the loop now.

I think doing loop here is a bit too much: we should just focus on a most likely case here and leave edge-cases out. The fidelity requirement is somewhat lower here.

So just go back to only managing attributes on the found elem?

I think he's saying the for attr in attrs loop is too much. We only care about name and id, nothing else.

@dvoytenko can you clarify?

Both:

It's enough to just have RestoreElementAttributesDef be {element, attrName, attrValue} and thus you can just take care of everything with a single loop.

We could even simplify this and only look for a single first element that matches either id search or name search. It seems to me that if there are several competing elements - we could live with that. But, obviously, a more foolproof solution is good as well :)

mkhatib · 2016-10-03T22:31:57Z

PTAL 👀

dvoytenko · 2016-10-03T22:42:11Z

src/document-click.js

@@ -14,11 +14,14 @@
 * limitations under the License.
 */

-import {closestByTag} from './dom';
+import {
+    closestByTag,


mkhatib · 2016-10-04T00:11:35Z

So @dvoytenko found few problems with this solution,

Breaking CSS styling (because of removing of ID attr)
Causing relayout with ID changes

I am exploring other solutions, one being calling scrollIntoView twice, once normally and once in a 0-timeout callback - the first is basically to override the browsers scroll right away and the timed out call to actually do the actual scroll into view.

viewport.scrollIntoView(elem);
setTimeout(() => viewport.scrollIntoView(elem), 0);

muxin · 2016-10-04T01:56:36Z

Why can't we use replaceState again? I saw a post about disabling default page scrolling by using replaceState

mkhatib · 2016-10-04T04:46:38Z

src/document-click.js

    viewport./*OK*/scrollIntoView(elem);
+    setTimeout(() => viewport./*OK*/scrollIntoView(elem), 0);


@dvoytenko here's the updated approach.

Can we use timer.delay here (which uses a micro task for 0 delays)?

I'm afraid it actually won't work in microtask :( We probably need to do it in its own event loop. That being said, timer.delay is a good idea, but probably with time 1?

mkhatib · 2016-10-04T04:47:24Z

src/document-click.js

-
-  // Push/pop history.
-  history.push(() => {
-    win.location.replace(`${curLoc.hash || '#'}`);


@dvoytenko dropping this to fix #4184 See discussion there. Let me know what do you think about this?

Don't yet understand why this is being dropped. Can you re-sum this up in either thread? This exists so that users can click on back button and get to normal state, especially given the :target support.

Don't yet understand why this is being dropped.

Agree, keeping this is a must.

OK, as I replied to that bug, I want to just focus on fixing this bug now. So I'll bring this line back and stick with the timer fix. And will address the other issue in a followup PR.

mkhatib · 2016-10-04T04:47:44Z

@dvoytenko PTAL 👀

Please ignore tests - outdated.

dvoytenko · 2016-10-04T21:57:30Z

@muxin When in iframe, replaceState may break parent history stack and definitely breaks :target selector.

mkhatib · 2016-10-04T23:05:37Z

Ok Updated PR now only addresses #5309 bug. Not the sidebar bug.

mkhatib · 2016-10-04T23:05:53Z

Please PTAL 👀

dvoytenko · 2016-10-04T23:23:06Z

src/document-click.js

+    elem = (ampdoc.getRootNode().getElementById(hash) ||
+        // Fallback to anchor[name] if element with id is not found.
+        // Linking to an anchor element with name is obsolete in html5.
+        ampdoc.getRootNode().querySelector(`a[name=${escapedHash}]`));


This CSS selector here also needs " I believe. Please add a test for search of an element with name with a space or such.

mkhatib · 2016-10-04T23:59:26Z

Done 👀 PTAL

dvoytenko · 2016-10-05T00:00:34Z

LGTM on my side.

mkhatib added the NEEDS REVIEW label Oct 3, 2016

mkhatib assigned dvoytenko and camelburrito Oct 3, 2016

Fix internal/local navigation links scrolling

bcdb1c9

jridgewell requested changes Oct 3, 2016

View reviewed changes

mkhatib commented Oct 3, 2016

View reviewed changes

dvoytenko suggested changes Oct 3, 2016

View reviewed changes

mkhatib force-pushed the debug-doclick branch from cacea06 to 94bfc04 Compare October 3, 2016 21:51

Address comments

834317d

mkhatib force-pushed the debug-doclick branch from 94bfc04 to 834317d Compare October 3, 2016 21:56

jridgewell reviewed Oct 3, 2016

View reviewed changes

mkhatib added 3 commits October 3, 2016 15:06

quote name

7ff7b45

Add tests

29a763b

escape css hash

26d32dc

fix call

e6c28c3

dvoytenko suggested changes Oct 3, 2016

View reviewed changes

mkhatib added 2 commits October 3, 2016 21:44

use timeout

40b03f2

indent

116c2f7

mkhatib commented Oct 4, 2016

View reviewed changes

update pr

ae1078f

fix type

2ad4513

dvoytenko suggested changes Oct 4, 2016

View reviewed changes

mkhatib added 2 commits October 4, 2016 16:26

fix css selector.

b8d39a0

add test

b0e9660

dvoytenko approved these changes Oct 5, 2016

View reviewed changes

jridgewell approved these changes Oct 5, 2016

View reviewed changes

mkhatib merged commit afbf063 into ampproject:master Oct 5, 2016

mkhatib deleted the debug-doclick branch October 5, 2016 20:01

dreamofabear pushed a commit to dreamofabear/amphtml that referenced this pull request Oct 6, 2016

Fix internal/local navigation links scrolling (ampproject#5381)

1642e8a

dreamofabear pushed a commit to dreamofabear/amphtml that referenced this pull request Oct 12, 2016

Fix internal/local navigation links scrolling (ampproject#5381)

47be39d

mityaha pushed a commit to ooyala/amphtml that referenced this pull request Nov 30, 2016

Fix internal/local navigation links scrolling (ampproject#5381)

3325426

		viewport./OK/scrollIntoView(elem);
		setTimeout(() => viewport./OK/scrollIntoView(elem), 0);

Fix internal/local navigation links scrolling #5381

Fix internal/local navigation links scrolling #5381

Conversation

mkhatib commented Oct 3, 2016 • edited

mkhatib commented Oct 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhatib commented Oct 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhatib commented Oct 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhatib commented Oct 4, 2016

muxin commented Oct 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhatib commented Oct 4, 2016

dvoytenko commented Oct 4, 2016

mkhatib commented Oct 4, 2016

mkhatib commented Oct 4, 2016

dvoytenko Oct 4, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhatib commented Oct 4, 2016

dvoytenko commented Oct 5, 2016

mkhatib commented Oct 3, 2016 •

edited

dvoytenko Oct 4, 2016 •

edited