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.

Files: 
CommentFileSizeAuthor
#19 openid-link-2-6.x.patch1.43 KBdanillonunes
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#16 openid-link-2.patch1.39 KBc960657
PASSED: [[SimpleTest]]: [MySQL] 33,644 pass(es).
[ View ]
#11 openid-link-1.patch2.67 KBc960657
PASSED: [[SimpleTest]]: [MySQL] 24,804 pass(es).
[ View ]
#5 openid.patch632 bytesdanillonunes
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in openid_9.patch.
[ View ]
#3 openid_link.patch1.07 KBPancho
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch openid_link.patch.
[ View ]

Comments

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.

Title:Login with OpenID link has mis-encoding of # and doesn't degradeLogin 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?

Status:Active» Needs review
StatusFileSize
new1.07 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch openid_link.patch.
[ View ]

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.

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?

StatusFileSize
new632 bytes
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in openid_9.patch.
[ View ]

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.

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.

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.

Subscribing

Patch #5 works well for me

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

Version:6.x-dev» 7.x-dev
StatusFileSize
new2.67 KB
PASSED: [[SimpleTest]]: [MySQL] 24,804 pass(es).
[ View ]

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.

Status:Needs work» Needs review

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

Priority:Minor» Normal

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

Subscribing

Version:7.x-dev» 8.x-dev
StatusFileSize
new1.39 KB
PASSED: [[SimpleTest]]: [MySQL] 33,644 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Tested

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.

Status:Needs work» Needs review
Issue tags:-needs backport to D6
StatusFileSize
new1.43 KB
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

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.

Issue summary:View changes

summary added by webchicks request

Issue summary:View changes

added blockquote tag

Issue summary:View changes

fixed block quote again

Issue summary:View changes

changed proposed solution

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.

Issue summary:View changes

moved original post