Problem/Motivation

OpenID login does not degrade with js turned off

Proposed resolution

I suggest turning the two links into real links (i.e. without return false in the click handler), linking to #openid and #, respectively. The openid.js file should check for location.hash on load and display the OpenID login field, if the user accessed the #openid URL directly. This allows users to bookmark the page with the visible OpenID login field.

Remaining tasks

patch needs to be tested

User interface changes

set open-id links as real external links

API changes

'data' => l(t('Log in using OpenID'), '#')
changed to

'data' => l(t('Log in using OpenID'), '#openid-login', array('external' => TRUE))

and

'data' => l(t('Cancel OpenID login'), '#')
to
'data' => l(t('Cancel OpenID login'), '#', array('external' => TRUE))

javascript now looks for openid-login hash

if ($('#edit-openid-identifier').val() || location.hash == '#openid-login')

Original post

As per title, the "login with openid" link is http://example.com/%23 --> the %23 is a weirdly encoded # sign.

And, looking at this, this means that people with JavaScript turned off can't login with openid. Suggestion is that in the absence of JavaScript, it gets turned into a link that goes to http://example.com/openid/login which will contain an openid-only login.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

walkah’s picture

Status: Active » Closed (works as designed)

Did you try with JS off? it absolutely does degrade... the link doesn't show with JS off, and the form field is displayed.

John Morahan’s picture

Title: Login with OpenID link has mis-encoding of # and doesn't degrade » Login with OpenID link has mis-encoding of #
Priority: Normal » Minor
Status: Closed (works as designed) » Active

It does degrade. But that %23 can't be by design, can it?

Pancho’s picture

Status: Active » Needs review
FileSize
1.07 KB

No, that's not by design, but there seems to be no work-around as we want to keep using the l()-function. The alternative would be without a hash, then it's at least not distorted. See my patch.

John Morahan’s picture

Do we even need l() here? From http://api.drupal.org/api/function/l/6 -

This function correctly handles aliased paths, and allows themes to highlight links to the current page correctly, so all internal links output by modules should be generated by this function if possible.

We don't need these features here if we simply want <a href="#">. Could we just output that directly?

danillonunes’s picture

FileSize
632 bytes

The trouble with this bug is no detected by who doesn't have javascript, but by who has js and are accessing a slow site. As the behaviors are not appended until the DOM is ready, if you click the link when the page is loading you is redirected to an 404 page.

Patch #3 don't fix totally since if you click the link before the page finish loading, your browser refresh the page... so... you need to wait it load again to login.

I make a patch using a solution proposed in api.drupal.org to create hash links with l() function.

c960657’s picture

I suggest turning the two links into real links (i.e. without return false in the click handler), linking to #openid and #, respectively. The openid.js file should check for location.hash on load and display the OpenID login field, if the user accessed the #openid URL directly. This allows users to bookmark the page with the visible OpenID login field.

Irous’s picture

I've just started getting the http://example.com/%23 problem when trying to log in via openID on my site. I've realised that this started happening when I enabled the "Optimize JavaScript files" in the Performance (caching) options in admin. Once I turned javascript caching off again, openID was working fine.

srobert72’s picture

Subscribing

srobert72’s picture

Patch #5 works well for me

srobert72’s picture

Drupal 6.17 released today.
I repatched manually openid.module with patch #5.
All works with Drupal 6.17

c960657’s picture

Version: 6.x-dev » 7.x-dev
FileSize
2.67 KB

This implements (almost) what I suggested in #6. This will make the link works as a link if you right-click and chose "Open in new window...".

Or is this over-thinking?

Status: Needs review » Needs work

The last submitted patch, openid-link-1.patch, failed testing.

c960657’s picture

Status: Needs work » Needs review

#11: openid-link-1.patch queued for re-testing.

Daniel Norton’s picture

Priority: Minor » Normal

Bumped priority. Creating invalid links is definitely not a “cosmetic” issue.

wojtha’s picture

Subscribing

c960657’s picture

Version: 7.x-dev » 8.x-dev
FileSize
1.39 KB
wojtha’s picture

Status: Needs review » Reviewed & tested by the community

Tested

Dries’s picture

Version: 8.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D6

Committed to 7.x and 8.x.

Sounds like this needs to get backported to Drupal 6 so I'm marking this as 'needs work' against 6.x-dev.

danillonunes’s picture

Status: Needs work » Needs review
Issue tags: -Needs backport to D6
FileSize
1.43 KB

Here is the patch for 6.x.

To test it, I just wrap the Drupal.attachBehaviors(this); in misc/drupal.js:276 with a setTimeout(function() { Drupal.attachBehaviors(this); }, 5000);, in order to simulate a slow execution of behaviors.

jasonrichardsmith@gmail.com’s picture

Issue summary: View changes

summary added by webchicks request

jasonrichardsmith@gmail.com’s picture

Issue summary: View changes

added blockquote tag

jasonrichardsmith@gmail.com’s picture

Issue summary: View changes

fixed block quote again

jasonrichardsmith@gmail.com’s picture

Issue summary: View changes

changed proposed solution

c960657’s picture

Tested and reviewed.

I wonder whether we should skip the || location.hash == '#openid-login' part from the D6 patch? It's a new feature that might surprise someone.

c960657’s picture

Issue summary: View changes

moved original post

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.