[PAY TALHA] [HOLD for payment 2023-09-27] [$1000] ANDROID CHROME - Split Bill: LHN does not close if clicking on the Workspaces link

kavimuru commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

Go to https://help.expensify.com/articles/playbooks/Expensify-Chat-Playbook-for-Conferences on mobile device.
Open LHN and click on any step.
Notice the LHN closes and page scrolls down to relevant section.
Now go to https://help.expensify.com/hubs/split-bills.
Open LHN and click on Workspaces.
Notice the LHN does not close and nothing happens.
Expected Result:

In case of https://help.expensify.com/hubs/split-bill, clicking on links in the LHN which points to some section on the same page, the LHN should close and the screen should be scrolled down to relevant section.

Actual Result:

In case of https://help.expensify.com/hubs/split-bill, clicking on links in the LHN which points to some section on the same page, the LHN did not close.

Note: This seems to occur on the Workspaces link (the other 3 links seem fine).

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

[ ] Android / native
[x] Android / Chrome
[ ] iOS / native
[x] iOS / Safari
[ ] MacOS / Chrome / Safari
[ ] MacOS / Desktop

Version Number: 1.3.47-2 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/43996225/887fc734-4577-4317-a9b3-2d1318d247c2

https://github.com/Expensify/App/assets/43996225/a6ccfad3-b76d-40d3-938a-8062b00f0263

Expensify/Expensify Issue URL: Issue reported by: @Talha345 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690460581911079

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01b88583567474c0d1
Upwork Job ID: 1685897339918934016
Last Price Increase: 2023-09-11
Automatic offers:

abdulrahuman5196 | Reviewer | 26631441
Talha345 | Contributor | 26631442
Talha345 | Reporter | 26631443

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[x] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[x] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[x] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

Talha345 commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

In help docs,the LHN does not close when clicking on some link that points to some section on the same page.This issue is only present on https://help.expensify.com/hubs/split-bills while other pages like https://help.expensify.com/articles/playbooks/Expensify-Chat-Playbook-for-Conferences do not have this issue.

What is the root cause of that problem?

The RC is that there is no implementation to take care of this issue on this page.The issue does not exist on other pages because the same page links in the LHN on other pages are populated using the following code. Here we can clearly see that toggleHeaderMenu function is called whenever a link is clicked which was populated using tocbot.

Let us consider the following 2 links:

If we open both of these links on a mobile device and open the sidebar we see some links under the selected link.See images below.

Now if you try to click on any of the sub links in 2nd image with the Step 1/2 etc, you will see that the sidebar closes and the page is scrolled down to the correct section whereas this behaviour is not present in the links in the 1st image specifically "Paying Friends" and "Workspaces".

The function toggleHeaderMenu is responsible for closing the sidebar on mobile devices if a link is clicked which points to some section on the same page.

The reason, the sublinks in the link 2 mentioned above work correctly is because those sublinks were populated via a JS library called TocBot which is used to create Table of Contents.This can be seen here.If you look, we are calling the toggleHeaderMenu function on the onclick event within this codeblock. This means that whenever a link in the sidebar which is populated using TocBot is clicked,the toggleHeaderMenu function is executed which closes the sidebar on mobile devices.

Now the reason why we still have this issue with sublinks on link 1 above is due to the fact that these sublinks were not populated using TocBot and the reason for this is that these sublinks are not TOC.For example, the links "Paying Friends" and "Workspaces" have href pointing to some section of the same page whereas the links "Request And Split Bills" and "The Free Plan" are links to some other pages.Since the function toggleHeaderMenu is not called on the click of "Paying Friends" and "Workspaces", this issue occurs.

What changes do you think we should make in order to solve the problem?

There are 2 solutions to fix this issue:

We implement a generic JS in the main.js file to take care of all navigation links in the help docs LHN. That can be done as follows:

function isSamePageScrollLink(link) { const href = link.getAttribute("href"); if (href.startsWith("#") && !!document.getElementById(href.slice(1))) { toggleHeaderMenu(); } }

const lhnContent = document.getElementById('lhn-content'); lhnContent.addEventListener('click', (event) => { const clickedLink = event.target; if (clickedLink) { isSamePageScrollLink(clickedLink); } });


Using this we add an event listener to any link in the LHN and upon click we can check if the href attribute of a particular link points to some section of the same page using `isSamePageScrollLink` and if so we trigger `toggleHeaderMenu`.

Furthermore, this would require to remove the `toggleHeaderMenu` method call in the TocBot onclick event [here](https://github.com/Expensify/App/blob/main/docs/assets/js/main.js#L198-L200) otherwise it will be called twice for TocBot links. This way we will have a generic code that deals with all navigation links in the LHN help docs.

2. The second solution would be to just implement the sublinks on https://help.expensify.com/hubs/split-bills using Tocbot but this does not make much sense imo because we can see that there are sublinks below 'Paying Friends' and 'Workspaces' links which are not actually links to the same page and redirect to some other page and therefore are not necessarily TOC.

### What alternative solutions did you explore? (Optional)

N/A

kadiealexander commented 1 year ago

Swapping with JLi (we have an agreement to swap android/iOS issues)

jliexpensify commented 1 year ago

I can verify this is a bug, but only for the Workspaces link. I would encourage more testing though, this is what I'm seeing on a Pixel 3a.

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01b88583567474c0d1

melvin-bot[bot] commented 1 year ago

Current assignee @jliexpensify is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @abdulrahuman5196 (External)

Talha345 commented 1 year ago

@jliexpensify This bug exists only for Paying Friends and Workspaces links because both of these links have href attributes pointing to the same page. Also I see that you added android chrome to the task title but this issue is also on IOS safari on mobile devices because we do not have any implementation to cater this.You can check my proposal for further details.

abdulrahuman5196 commented 1 year ago

Reviewing today

Talha345 commented 1 year ago

@abdulrahuman5196 did you get a chance to review this?

jliexpensify commented 1 year ago

@Talha345 thanks for the context - just as an FYI, the GH's are original created by the QA team based off the reporter's findings. So if it's also an iOS problem, I'd encourage you to add that in your initial report so it can be recorded.

abdulrahuman5196 commented 1 year ago

@jliexpensify Are we actually fine to report and to work on the help pages? Asking this because, I haven't personally seen issues raised on help pages. I could be wrong as well, but wanted to confirm

Talha345 commented 1 year ago

s raised on help pages. I could b

@abdulrahuman5196 The code base for the help docs is in the same repository and I have actually worked on an issue within the help docs before too.Here is the link for that issue: https://github.com/Expensify/App/issues/22014

jliexpensify commented 1 year ago

Yep @abdulrahuman5196 I'm pretty sure we can hire Contributors to work on the Help pages. I would say find a proposal that works and we'll get an Engineer assigned, and they can have the final say!

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

jliexpensify commented 1 year ago

@abdulrahuman5196 how's @Talha345 's proposal looking?

Talha345 commented 1 year ago

Hi @abdulrahuman5196,do you have any update as this is a pretty straightforward task in my opinion?

jliexpensify commented 1 year ago

Bump @abdulrahuman5196 please review the proposal, thanks!

abdulrahuman5196 commented 1 year ago

Reviewing now

abdulrahuman5196 commented 1 year ago

On @Talha345 's proposal here https://github.com/Expensify/App/issues/23810#issuecomment-1656068129 I am not able to fully understand the proposal.

The issue does not exist on other pages because the same page links in the LHN on other pages are populated using the following code. Here we can clearly see that toggleHeaderMenu function is called whenever a link is clicked which was populated using tocbot.

What does this mean? Do we say that toggleHeaderMenu is not properly working or we are saying it should have never called or any other. I am not able to understand this clearly.

The second solution would be to just implement the links on https://help.expensify.com/hubs/split-bills using Tocbot

What do you mean by links here? Could you share references of those?

We implement a generic JS to take care of all such links in the help docs.That can be done as follows:

Where are we going to add the mentioned code? And how is that impacting?

@Talha345 Could you add more detailed information on the proposal?

Talha345 commented 1 year ago

Hi @abdulrahuman5196 , Proposal updated.

I think you did not understand my proposal clearly because the links to the code in my proposal had deviated because new code was added in the code base and the line numbers were not same anymore.I have updated those too.Let me know if you have any further questions.

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

jliexpensify commented 1 year ago

Bumop @abdulrahuman5196 for review!

getusha commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

LHN does not close if clicking on workspaces link.

What is the root cause of that problem?

The links don't have any event listeners to close the LHN when clicked, which is the better user experience. This issue happens when users go to different parts of the same page without actually changing the whole page.

For example, from https://help.expensify.com/hubs/split-bills to https://help.expensify.com/hubs/split-bills#workspaces Here, the page doesn't fully change, it just moves to a specific section on the page.

When the page reloads, left-hand navigation closes. But in the situation mentioned above, where there's no complete page reload, this doesn't happen.

What changes do you think we should make in order to solve the problem?

Solution 1 We need to invoke the toggleHeaderMenu function when the hashchange event occurs. This event is triggered when there's a change in the fragment identifier of the URL, which is the portion of the URL that starts with the # symbol. which is our current case. You can find additional information on this event here.

We will add

window.addEventListener('hashchange', toggleHeaderMenu);
// or
window.onhashchange = toggleHeaderMenu;

below here: https://github.com/Expensify/App/blob/7f13cdd9f5ade3aed50827084aa49b5e3a6496e1/docs/assets/js/main.js#L204

Solution 2

invoke toggleHeaderMenu function when the popstate event occurs. The popstate event of the Window interface is fired when the active history entry changes while the user navigates the session history. this is also working perfectly for out case. We will add

window.addEventListener('popstate', toggleHeaderMenu);
// or
window.onpopstate = toggleHeaderMenu;

https://github.com/Expensify/App/assets/75031127/331be74d-67be-4bf6-9cd8-47707e8cce19

What alternative solutions did you explore? (Optional)

N/A

cc @abdulrahuman5196

melvin-bot[bot] commented 1 year ago

@jliexpensify, @abdulrahuman5196 Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 1 year ago

@jliexpensify, @abdulrahuman5196 Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

jliexpensify commented 1 year ago

Bump @abdulrahuman5196 for reviews!

Talha345 commented 1 year ago

@jliexpensify Can you ping @abdulrahuman5196 in Slack maybe because he has been MIA?

abdulrahuman5196 commented 1 year ago

Reviewing now

abdulrahuman5196 commented 1 year ago

@Talha345

Thanks for the updated proposal. Now I have some idea on the proposal made.

The second solution would be to just implement the sublinks on https://help.expensify.com/hubs/split-bills using Tocbot but this does not make much sense imo because we can see that there are sublinks below 'Paying Friends' and 'Workspaces' links which are not actually links to the same page and redirect to some other page and therefore are not necessarily TOC.

Why do you think this is not best? Any drawbacks to this?

IMO I think this is better, the reason is even in the code comments it is mentioned all the TOC links should come here on the onclick function. I think this case should extend to sublinks as well and clicking sublinks also should trigger this.

https://github.com/Expensify/App/blob/f32b695764ded61bdd771e2d2504e0dd6679ce8a/docs/assets/js/main.js#L210-L217

Do you have references on how we can we achieve this?

Talha345 commented 1 year ago

IMO I think this is better, the reason is even in the code comments it is mentioned all the TOC links should come here on the onclick function. I think this case should extend to sublinks as well and clicking sublinks also should trigger this.

@abdulrahuman5196 You are completely right.The code you mentioned above also gets executed when a click on a sublink is triggered.

The reason I mentioned that this does not make sense is that if you see the links in the screenshot below, only "Paying Friends" and "Workspaces" links are pointing to the same page but the sublinks "Request and Split Bills" and "The Free Plan" redirect to some other page, therefore this navigation list is not a Table of Content because TOC usually lists all the headings on a particular page https://en.wikipedia.org/wiki/Table_of_contents.

On the other hand, if you see the links in the screenshot below, all the links are headings to some section of the same page and are therefore TOC for this particular page.

The second solution is also not really feasible on how the current docs are implemented.This will require a lot of changes to be done at various places.The LHN is generated using the file lhn-template.html for HUB pages which generates the links in the navigation on the basis of routes.yml file which is generated by createDocsRoutes which generates these routes on the basis of how the files are structured under articles directory. Only the ARTICLES pages navigation links are generated using TocBot.

Also if you checkout the documentation. of Tocbot, it says: "Tocbot builds a table of contents from headings in an HTML document." and in our case, we do not have "Request and Split Bills" and "The Free Plan" headings on the page that we are discussing.

Another reason I don't like this solution is if for instance a new section is added to the help docs in the future and that particular section is structured in a different way, then this bug will again occur on that page.

My first solution is a generic solution and will cater all the links ever added to the LHN.

getusha commented 1 year ago

@abdulrahuman5196 any comments on my proposal? in case you missed it🙂 https://github.com/Expensify/App/issues/23810#issuecomment-1680676503

melvin-bot[bot] commented 1 year ago

@jliexpensify @abdulrahuman5196 this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Decide whether any proposals currently meet our guidelines and can be approved as-is today
If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

melvin-bot[bot] commented 1 year ago

Current assignee @abdulrahuman5196 is eligible for the Internal assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

Current assignee @jliexpensify is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

Current assignee @abdulrahuman5196 is eligible for the External assigner, not assigning anyone new.

jliexpensify commented 1 year ago

Still looking at proposals - bumping @abdulrahuman5196 to address comments

abdulrahuman5196 commented 1 year ago

Reviewing now

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

abdulrahuman5196 commented 1 year ago

@Talha345

The reason I mentioned that this does not make sense is that if you see the links in the screenshot below, only "Paying Friends" and "Workspaces" links are pointing to the same page but the sublinks "Request and Split Bills" and "The Free Plan" redirect to some other page, therefore this navigation list is not a Table of Content because TOC usually lists all the headings on a particular page https://en.wikipedia.org/wiki/Table_of_contents.

This is technically true, but AFAIK there are multiple pages in the docs, whose TOC points to different pages. So this might not hold true for us logically.

Only the ARTICLES pages navigation links are generated using TocBot.

Not sure what you mean by here.

abdulrahuman5196 commented 1 year ago

@abdulrahuman5196 any comments on my proposal? in case you missed it🙂 #23810 (comment)

@getusha I did review this proposal, but it was similar to the proposal here https://github.com/Expensify/App/issues/23810#issuecomment-1656068129. Just using different listener IMO.

getusha commented 1 year ago

@getusha I did review this proposal, but it was similar to the proposal here #23810 (comment). Just using different listener IMO.

The proposal you mentioned suggested to add click listener to every link with a loop, and i suggested to add window listener to check if the url is changed or not and it's one liner solution, which is completely different.

Talha345 commented 1 year ago

This is technically true, but AFAIK there are multiple pages in the docs, whose TOC points to different pages. So this might not hold true for us logically.

@abdulrahuman5196 can you provide some examples because I do not think that is true because the TOC is generated from the heading tags within the HTML of that page when using TocBot!

Not sure what you mean by here.

What I mean is that https://help.expensify.com/hubs/split-bills is a HUBS page as it belongs to the main 4 sections in the LHN which we call as HUBS.It also has hubs in the URL itself.

On the other hand, the pages where the LHN is generated using TocBot are ARTICLES pages like https://help.expensify.com/articles/playbooks/Expensify-Chat-Playbook-for-Conferences.

This can be seen here.

Also my first solution is a totally generic solution and will also act as a single source for toggling the LHN using the toggleHeaderMenu. Not sure why aren't we just going further with that.

abdulrahuman5196 commented 1 year ago

@Talha345 @getusha

I still feel adding a listener and closing the LHN based on the url is workaround solution. (I will check with others if this is the only path)

Let me gather up what I have understood till now and let me know if I am wrong about anything.

1) There are pages which is using TocBot And there are pages which are not 2) And the issue page doesn't use TocBot and so doesn't auto close the menu. 3) It is not feasible to implement TocBot to pages which currently doesn't have.

So the proposals target to add listeners in some form to detect page/url changes and close the header menu.

getusha commented 1 year ago

@abdulrahuman5196 i believe the reason the LHN is closing when clicking on other links, is because of a full page reload, but in this case it is in-page link which will not cause a full page reload.

Talha345 commented 1 year ago

Let me gather up what I have understood till now and let me know if I am wrong about anything.

There are pages which is using TocBot And there are pages which are not

And the issue page doesn't use TocBot and so doesn't auto close the menu.

It is not feasible to implement TocBot to pages which currently doesn't have.

So the proposals target to add listeners in some form to detect page/url changes and close the header menu.

@abdulrahuman5196 Your understanding is correct. However, my proposal is to add an event listener for the click event and not to detect page/url changes.

Also, how this my proposal a workaround.It is basically implementing a functionality that is required and not present currently.When a user clicks a link in the LHN and the link points to the same page, we want to close the LHN and that's what my proposal is doing.If you look at the main.js file, we already have such listeners for other required functionality.

abdulrahuman5196 commented 1 year ago

@getusha @Talha345

I was trying both your solutions. For some reason both doesn't work for me. (Ran both solutions separate)

And just ran following references here https://github.com/Expensify/App/tree/main/docs

But I am still seeing this issue in android simulator. Let me know if I am missing something.

If you look at the main.js file, we already have such listeners for other required functionality.

Yes true. Seems I need to re-evaluate if my workaround mention is right. But anyways I will try if the solution is working then, will proceed accordingly.

https://github.com/Expensify/App/assets/46707890/a43caac1-51bd-400e-a0e2-8c1a812ec77f

Talha345 commented 1 year ago

@getusha If you do not mind, I can provide some feedback on why your proposed solution is not very efficient and can also lead to further bugs. What you are suggesting is to add event listeners on hashchange or popstate event.If we do this and a user is on a page and goes to the URL and updates the fragment identifier manually, the LHN will open automatically if it is closed and vice versa.

Secondly, your solution will intefere with the pages implemented via TocBot and the toggleHeaderMenu will be called twice for those pages which will just close and open the LHN again.

Expensify / App