WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#10939 closed defect (bug) (fixed)

browser with iframes off is left a blank lightbox

Reported by: hakre Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9
Component: Accessibility Keywords: has-patch commit
Focuses: Cc:

Description

post edits screen's lightbox (upload, images, gallery) is just only blank when a browser does not support iframes.

a message should be displayed in that case (<noframes> etc.) sothat a user does not think this is an error (white screen of death) but just a configuration issue.

Attachments (2)

10939.patch (3.9 KB) - added by hakre 6 years ago.
First patch, still containing temporary code because of the visual editor is broken in my head testbed right now.
10939.2.diff (3.2 KB) - added by nacin 5 years ago.
thickbox accessibility text

Download all attachments as: .zip

Change History (14)

@hakre6 years ago

First patch, still containing temporary code because of the visual editor is broken in my head testbed right now.

comment:1 @hakre6 years ago

I double-checked the codebase now and there are no changes against SVN, disabled cache, deleted all cookies and enabled frames in firefox again. Visual Editor is still broken. I need to continue when this is fixed.

comment:2 @nacin6 years ago

  • Keywords needs-patch added
  • Version changed from 2.8.4 to 2.9

JavaScript iframes appear in scriptaculous, thickbox, and tinymce. I agree those should have warnings as functionality is clearly lost.

I do not think we need a message in plugin-editor.php, plugins.php or class-wp-upgrader.php. In those instances, the fatal error message of a plugin is displayed in an iframe, beneath a warning message saying the plugin could not be activated because of an error. I'd call the use of iframes here progressive enhancement. You don't notice that there was lost functionality when iframes are disabled.

Or, if the message is warranted, then the feedback should be something like "We'd show you the fatal error but you do not have iframes enabled in your browser."

Also, with regards to your patch, the implementation is off. <noframes> is for framesets, not inline frames, and you can have one but not the other enabled in a browser. To provide alternate content for an iframe, you'd provide it between the iframe tags, such as <iframe>Sorry, no iframes.</iframe>.

I'd also suggest something like this for the text:

This feature requires inline frames. You have iframes disabled or your browser does not support them.

comment:3 @nacin6 years ago

  • Milestone changed from 2.9 to 3.0

I'll do a patch for 3.0.

@nacin5 years ago

thickbox accessibility text

comment:4 @nacin5 years ago

  • Keywords has-patch added; needs-patch removed

Patch for thickbox is attached.

Not sure about the others, as I said. Don't find this necessary (and I would find the generic warning confusing) for plugin activations. (The other ones are scriptaculous and tinymce.)

comment:5 follow-up: @Denis-de-Bernardy5 years ago

  • Keywords commit added

Patch applies fine and works, as in it doesn't prevent thickbox/the uploader from working. no idea how to test this on a browser that has no frames, though. :-P

dev review needed here -- suggesting we get this checked in if there is any interest, or close.

comment:6 in reply to: ↑ 5 @nacin5 years ago

Replying to Denis-de-Bernardy:

no idea how to test this on a browser that has no frames, though. :-P

You can turn off iframes in Opera pretty easily, that's how I tested it. It isn't pretty, but it doesn't leave you with a white thickbox, so it's enough for me.

comment:7 @azaozz5 years ago

10939.2.diff looks good but will break plugins that load thickbox.js directly (if any still do that). Since this is an edge case, perhaps we could hardcode the error messages in thickbox.js.

comment:8 @Denis-de-Bernardy5 years ago

@Andrew: I fail to see where it'll break plugins. Here's one that continues to work just fine with the patch applied:

http://wordpress.org/extend/plugins/auto-thickbox/

comment:9 follow-up: @azaozz5 years ago

Seen some that load directly TB (not with wp_enqueue_script) so the thickboxL10n will be undefined. Or perhaps it was themes.

comment:10 in reply to: ↑ 9 ; follow-up: @nacin5 years ago

Replying to azaozz:

Seen some that load directly TB (not with wp_enqueue_script) so the thickboxL10n will be undefined. Or perhaps it was themes.

Hmm, what about other instances of thickboxL10n?

comment:11 @azaozz5 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [12677]) Show error message in TB when iframes support is turned off in the browser, props nacin, fixes #10939

comment:12 in reply to: ↑ 10 @azaozz5 years ago

Replying to nacin:

Hmm, what about other instances of thickboxL10n?

Didn't even notice them... Seems I need more sleep.

Note: See TracTickets for help on using tickets.