T250253: update useNavigation to support stacked popups #210

medied · 2020-04-17T22:33:51Z

Phabricator Link: https://phabricator.wikimedia.org/T250253

Problem Statement

When we changed the article menu so that popups stack, in order to get the desired back key behavior when working on backspace key feature, a bug was introduced because useNavigation hook was not prepared to handle a specific case such as ArticleLanguage stacked on top of ArticleMenu. What was happening is that on every getAllElements() call, it was actually getting all elements from both the first and second popup. This was messing up navigation altogether.

Solution

At first I was trying to let it grab all elements from both popups and then slicing the array to handle the corresponding elements, but this approach wasn't really working correctly (branch T250253-article-menu-language-first-approach-idea for the curious). I opted instead for taking advantage of the elementsSelector argument to have the hook grab all the correct elements when ArticleLanguage is the origin. This required an update to getSelectedElement() so that it selects the correct element from the 'active' (latest) popup.

Note

Still a mystery to me why the first piece of code below works but the second one doesn't

allNavSelectedTrue.forEach(element => {
    console.log(‘element.attributes...', element.attributes)
    if (element.getAttribute(currentSelector)) {
       selectedElement = element
    }
})

const selectedElement = Array.from(allNavSelectedTrue).find(element => {
    return element.getAttribute(currentSelector) === true // returns null
})

medied · 2020-04-17T22:36:45Z

src/hooks/useNavigation.js

+    let selectedElement
+    allNavSelectedTrue.forEach(element => {
+      if (element.getAttribute(currentSelector)) {
+        selectedElement = element


I'm assuming there's always a nav-selected=true element with the corresponding currentSelector, but let's talk if that's not a safe assumption

hueitan · 2020-04-20T05:55:24Z

Haven't looked into the details yet, but when test on search page, navigation breaks.

medied · 2020-04-20T13:14:32Z

What behavior are you seeing? Seems to be working fine on my end

hueitan · 2020-04-20T15:57:03Z

You can follow useLinksNavigation, when doing the querySelectorAll, add the container instead of document to query elements under the container

wikipedia-kaios/src/hooks/useArticleLinksNavigation.js

Line 148 in b050fc9

const links = container.querySelectorAll(SUPPORTED_LINKS)

medied · 2020-04-21T02:50:49Z

Good idea. Updated PR to follow suit of this better approach, thanks for the feedback Huei 👍

medied · 2020-04-21T02:54:34Z

src/components/ArticleToc.js

@@ -14,7 +14,7 @@ export const ArticleToc = ({ items, currentAnchor, onSelectItem, close, closeAll

    if (item && item.title) {
      onSelectItem(item)
-      close()
+      closeAll()


Little (unrelated) bug I found along the way, a bonus fix

I don't advice doing "drive-by" fixes like that. If you do, make sure to provide steps to reproduce the issue that this is fixing.

The preferred way is to file a task describing the issue and link a one-line PR. Yes it takes longer, but it is so much more safer.

Makes sense

to reproduce the issue that this is fixing.

Issue was that when if you openned ArticleToc through the article menu, selecting a section there would route to correct section but the screen you saw was the article menu. This was because not all popups were closing, so this was introduced recently when we stacked popups from the article menu.

Will make sure to file a task next time.

hueitan · 2020-04-21T08:58:20Z

src/hooks/useNavigation.js


  const getSelectedElement = () => {
-    return document.querySelector('[nav-selected=true]')
+    const grandparentContainer = containerRef.current.parentNode.parentNode
+    return grandparentContainer.querySelector('[nav-selected=true]')


great, it works now!

two questions and comments

why grandparent, not just parent?

This fix. Although we will have the problem again if the grandparent or parent component use useNavigation, this is not our case as the container usually set to child components. I think we can ignore this now as this seems not a simple fix.

In the case of getSelectedElement, we can actully get a working behavior if we just return containerRef.current.querySelector('[nav-selected=true]’) directly instead. The only issue with that is a harmless TypeError if you click on (invisible) center softkey when input element is active with a ListView/RadioListView below:

Although we will have the problem again if the grandparent or parent component use useNavigation, this is not our case as the container usually set to child components. I think we can ignore this now as this seems not a simple fix

I think the latest commit addresses this too then, if I'm understanding you correctly

stephanebisson · 2020-04-21T13:22:46Z

src/hooks/useNavigation.js

@@ -4,10 +4,18 @@ import { useSoftkey } from 'hooks'
 export const useNavigation = (origin, containerRef, axis, elementsSelector = '[data-selectable]') => {
  const [current, setCurrent] = useState({ type: null, index: 0, key: null })

-  const getAllElements = () => document.querySelectorAll(elementsSelector)
+  const getAllElements = () => {
+    if (containerRef.current === undefined) {


I would like to understand when is containerRef.current undefined. I feel like querying document is never OK but I might be missing some context here.

containerRef.current is undefined when the app first loads the Search home screen. If we comment the if-statement here in line 8, this is stack trace that the console throws:

It’s from the setNavigation in the component's useEffect. This error messes up the loading of Search because softkeys don’t get set, the cursor doesn’t get set, and querying doesn’t work.

I am returning document.querySelectorAll(elementsSelector) in this case because querying document is what we were doing previously.

useEffect runs after the component has rendered and the refs are populated. However, in this case the ref is on the results list which doesn't exist until there are search results. What we are trying to do here with setNavigation(0) is to focus the input box.

Have 2 refs would fix that too. Or we could be more explicit with something along the lines of inputRef.current.focus()

stephanebisson · 2020-04-21T13:23:41Z

src/hooks/useNavigation.js

+      return document.querySelectorAll(elementsSelector)
+    }
+
+    const grandparentContainer = containerRef.current.parentNode.parentNode


Why do you provide a containerRef that is actually 2 levels down from the node we actually need. Can you explain how this works?

This is because we need the input element to also be part of the elements being returned by getAllElements. The input is also data-selectable and there are at least three cases where the input is found two levels away from where ref={containerRef} is being set: ArticleLanguage.js#L59, Language.js#L51, Search.js#L64

The idea then is to make the scope of the container big enough to contain all needed elements but small enough to also ignore other elements when popups are stacked. I tried a couple different approaches for this

For example moving the ref={containterRef} placement on level up in RadioListView and ListView, and then querying the containerRef parent node (instead of grandparent). This query does include the needed input and makes navigation work, but it messes up the screen scrolling code which also uses containerRef (useNavigation.js#L80). Scrolling code would have to be updated too.

Also tried setting ref={containterRef} actually two levels out, which means taking it outside of the RadioViewList/ListView component and into Search/Language/ArticleLanguage. But again this meant even further changes needed to get navigation to work.

So ultimately I’m proposing to grab the grandparent right from within getAllElements because it seems to be the least intrusive change that gets us the working behavior, correctly supporting navigation with stacked popups. Does this make sense? Can we think of a better way?

Thanks for the explanation. I remember now the issue with the input being conceptually part of the list for navigation but not for "scroll into view".

If there's a simple way to capture and use 2 refs, one for each purpose, I think that would be more clear but if not, a prominent comment explaining what's going on could do.

That's a great idea, what about this: 95900ec

Am I missing a component? Seems to be working fine

yeah, it looks good now 👍

jpita · 2020-04-24T05:23:39Z

found an issue:
1-open "gatwick airport" article in english
2-select the "quick facts" from the quick actions menu
3-select the first image (there's another bug here that I will create a ticket for)
BUG:
If you use the physical back button here, it goes back to the quick facts.
If you use the "close" button from the app, it goes back to the article.

IMO both buttons should have the same behaviour/call the same method.
There's a similar issue here

hueitan

It looks good to me now 👍
the code looks better to have two ref to split the container and list.

Update useNavigation to support stacked popups

3042582

medied commented Apr 17, 2020

View reviewed changes

medied added 2 commits April 20, 2020 18:55

Use closeAll to support stacked popup case

f082bf2

Better approach for useNavigation to support stacked popups

a0bdd93

medied commented Apr 21, 2020

View reviewed changes

Clean up

5be1870

hueitan reviewed Apr 21, 2020

View reviewed changes

stephanebisson reviewed Apr 21, 2020

View reviewed changes

Even better approach for useNavigation to support stacked popups

95900ec

hueitan approved these changes Apr 24, 2020

View reviewed changes

jpita merged commit a307408 into master Apr 27, 2020

jpita deleted the T250253-article-menu-language branch April 27, 2020 02:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T250253: update useNavigation to support stacked popups #210

T250253: update useNavigation to support stacked popups #210

medied commented Apr 17, 2020

medied Apr 17, 2020

hueitan commented Apr 20, 2020

medied commented Apr 20, 2020

hueitan commented Apr 20, 2020

medied commented Apr 21, 2020

medied Apr 21, 2020

stephanebisson Apr 21, 2020

medied Apr 21, 2020

hueitan Apr 21, 2020

medied Apr 21, 2020

medied Apr 22, 2020

stephanebisson Apr 21, 2020

medied Apr 21, 2020

stephanebisson Apr 21, 2020

stephanebisson Apr 21, 2020

medied Apr 21, 2020 •

edited

stephanebisson Apr 21, 2020

medied Apr 22, 2020 •

edited

hueitan Apr 24, 2020

jpita commented Apr 24, 2020

hueitan left a comment

T250253: update useNavigation to support stacked popups #210

T250253: update useNavigation to support stacked popups #210

Conversation

medied commented Apr 17, 2020

Problem Statement

Solution

Note

Choose a reason for hiding this comment

hueitan commented Apr 20, 2020

medied commented Apr 20, 2020

hueitan commented Apr 20, 2020

medied commented Apr 21, 2020

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

medied Apr 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medied Apr 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpita commented Apr 24, 2020

hueitan left a comment

Choose a reason for hiding this comment

medied Apr 21, 2020 •

edited

medied Apr 22, 2020 •

edited