Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Whether pathname includes base depends on whether there is an anchor (#foo) #589

Open
evanbattaglia opened this issue Oct 21, 2021 · 0 comments
Open

Whether pathname includes base depends on whether there is an anchor (#foo) #589

evanbattaglia opened this issue Oct 21, 2021 · 0 comments

Comments

@evanbattaglia
Copy link

I apologize if I missed an existing issue for this, or if this is expected behavior, but it seems surprising to me. #575 is similar but this is specifically around pathname and base URL. Assuming hashbang is false, if there is no hash symbol (anchor) in the document URL, the pathname is based on path so includes the base URL. In Context():

page.js/page.js

Line 1090 in 4f99916

this.pathname = _page._decodeURLEncodedURIComponent(~i ? path.slice(0, i) : path);

However, if there is a hash symbol (anchor), pathname depends on this.path, which does not have the base URL.

page.js/page.js

Line 1098 in 4f99916

this.path = this.pathname = parts[0];

I came across while tracking a bug in our code. Here is a gist which shows the behavior in isolation.

https://gist.github.com/evanbattaglia/2b76bf11fe8a18ac4dcef14f2138a2cc

URL: /index -> pathname /basic/
URL: /index#whoop -> pathname /
URL: /basic/about -> pathname /basic/about
URL: /basic/about#whoop -> pathname /about

The documentation states pathname is The pathname void of query string "/login"., which implies it should not have the base URL (/admin in the documentation).
instructure-gerrit pushed a commit to instructure/canvas-lms that referenced this issue Oct 26, 2021
@evanbattaglia
Fix broken "View App Configurations" link
4644fde 
Also fully remove `alreadyRendered`, which was added in 309e5fd but saw
its usage removed in 0bf0399.

Added PropType `baseUrl` to ensure that is sent; Gergich then required my
to add a PropType for `app`.

fixes INTEROP-7118
flag=none

The value of page.js's "pathname" is dependent on whether there is a
hash symbol (anchor) in the current URL. If there is no anchor, the
pathname contains (starts with) the base URL (e.g.
"/accounts/self/settings"). If there is an anchor, it does not contain
it. The problem is that sometimes, the clicking the Apps tab changed the
web browser's current URL to "/accounts/self/settings#tab-tools". As a
result, pathname was "", and the configurations URL, which is normally
"/accounts/self/settings/configurations", was then just
"/configurations". I have filed a bug report on page.js, but the project
doesn't seem terribly active.
visionmedia/page.js#589

This bug apparently cropped up with d07531a, before which we never
added a anchor onto the settings URL. This commit was never in prod, but
my guess is that the times we seen it in prod were when we first went to
beta to repro it, then simply got rid of the "beta." from the URL,
leaving canvas.instructure.com/accounts/self/setting#tab-tools, from
which we could repro the issue.

For the fix, "baseUrl" is already used successfully in AppDetails to
construct URLs.  I can't find any reason to rely on pathname, and
pathname is not used for any other purpose.

6e31dcf fixed a similar (same?) issue, but that was on an older version
of path.js which did not overwrite pathname if there was a hash symbol.
In the old version of path.js, "/accounts/self/settings#tab-tools" would
yield a pathname of "/accounts/self/settings#tab-tools" so we needed to
cut off the "#tab-tools". In the current version, it yields a pathname
of "".

Test plan:
- go to /accounts/self/settings
- click various tabs and try setting a couple options to make sure
  nothing is broken there.
- click the Apps tab. Note whether or not the browser's URL has changed
  to add "#tab-tools" or not. Within the Apps tab, click "View App
  Configurations", then back to "View App Center", then click
  a particular app (which adds "app/abcdef123-..." to the browser
  location) and click both "View App Configurations" and "View App
  Center" from there. Make sure all the links work.
- If your browser did add "#tab-tools" which clicking the tab, try
  disabling the "remember_settings_tab" feature flag and repeat the tests.
  If it didn't, check that that flag is on.
- go to "/accounts/self/settings#tab-tools". It should jump to the Apps
  tab. Check out that links again and make sure they all work. In
  particular make sure the "View App Configurations" button works when
  the browser URL has "#tab-tools"
- Try editing tools, clicking the info icon or disabling/enabling
  placements. Try to break stuff.
- The test plan from 6e31dcf says to use the keyboard, but that didn't
  do anything different for me. You can try it.
- Run all the above steps for the course settings page.
- Run all the above steps for the course details page
  (/courses/123/details).

Change-Id: I2d5cfdcbbb8f0cfd2d43b61b2b5eebf034c77062
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/276597
Tested-by: Service Cloud Jenkins <[email protected]>
Reviewed-by: Sean Scally <[email protected]>
QA-Review: Xander Moffatt <[email protected]>
Product-Review: Evan Battaglia <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@evanbattaglia