WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 3 weeks ago

#33704 assigned enhancement

Reduce reliance on wp_is_mobile()

Reported by: johnbillion Owned by: adamsilverstein
Milestone: Future Release Priority: normal
Severity: normal Version: 3.4
Component: General Keywords: has-patch needs-testing
Focuses: ui, javascript Cc:

Description

User agent sniffing is bad. wp_is_mobile() isn't very reliable. In core it's actually used to detect if the device is a touch device (so that, among other things, elements that normally appear on hover are shown persistently and JavaScript for touch functionality is loaded).

We should audit the use of wp_is_mobile() in core to see if replacing it in favour of client-side feature detection is possible. If so, wp_is_mobile() should be deprecated.

Reported bugs with wp_is_mobile(): #24541, #21667, #24252.

History: #20014

Attachments (2)

33704-focus.diff (5.4 KB) - added by akibjorklund 14 months ago.
33704-login.diff (1.7 KB) - added by akibjorklund 14 months ago.

Download all attachments as: .zip

Change History (29)

#2 @akibjorklund
14 months ago

I went through all the instances of calls to wp_is_mobile. It is called 34 times in 23 different files. I've roughly categorized the instances.

1. To determine if a JavaScript that automatically sets focus to a field should be written: 5 instances

Not sure if this is a good idea in general, but could be based on the viewport width just as well. At least makes sense to disable the feature on smaller screens as the virtual keyboard will take up valuable space.

2. To print out a mobile or ios CSS class: 5 instances

In /wp-admin/css there are 19 matches for .mobile and 13 for .ios. I did not look at those in detail, but probably most of them can be converted to use media queries. Adding mobile class to body might be required for backwards compatibility reasons, but this could be done with JavaScript just as well, we just need to determine what would be the features to test against. Plugins could have used the class for anything, at least in theory, so this could be hard.

3. To print out meta viewport tags: 2 instances

These are probably entirely unnecessary checks, those tags won't harm non-mobile browsers.

4. Related to upload capabilities: 4 instances

Some of these might be not relevant anymore, since they refer to old versions of iOS.

5. Related to which editor features are enabled: 5 instances

Editor's expandability, distraction free writing and help buttons are set based on is_wp_mobile. Also used to detect whether rich editing should be enabled.

6. To load jQuery UI Touch Punch: 6 instances

jQuery UI Touch Punch is a library that makes jQuery UI components to react to touch events. It is used to make meta boxes and other elements draggable, maybe something else. There are ways to detect touch capabilities on the client, but nothing 100% reliable. Probably better than user agent based though.

7. For video on the about page: 2 instances

This is for the streamlined updates animation in 4.6, not for the 4.6 main video. Probably used because there is no autoplay on iOS for video and that animation is not something you want to press play for and watch fullscreen. iOS 10 will allow silent autoplay and inline video, so that would work in the future without mobile detection if there are similar needs in the future. Not sure about Android though.

8. Customizer: 4 instances

class-wp-customize-manager.php and theme.php set some variables for mobile and ios to pass them to JavaScript. I found only one instance that uses those: customizer widgets do similar autofocus that category 1 instances do. There could be more uses for these and themes could use these variables as well.

9. Login shake: 1 instance

Need to investigate why this is not enabled for mobile devices. Maybe the animation was not smooth enough back then?

#

All of those might require a different approach. Should there be a ticket for each of those categories with individual patches or should there be one giant patch within this ticket?

Related: browser globals ($is_lynx, $is_edge, $is_chrome, etc.) are also determined from the User-Agent header and are just as evil. I hope no-one is using them to detect Netscape 4 anymore for example. This definitely is a topic for another ticket though.

Last edited 14 months ago by akibjorklund (previous) (diff)

#3 @johnbillion
14 months ago

  • Focuses javascript added
  • Keywords dev-feedback removed
  • Milestone changed from Awaiting Review to 4.7

Thank you for that, @akibjorklund, that's a big help. Let's see what we can tackle for 4.7.

#4 @Ipstenu
14 months ago

Just FYI, there are a lot of plugins calling this. I just started the search and I have over 80 before leaving the As.

#5 @curdin
14 months ago

Looking at the CSS a bit closer the use of .ios is usually to address browser quirks / performance. As @akibjorklund hinted at both .ios and .mobile appear to be replaceable by use of mediaqueries. Saying that if js will be used to inject .mobile we can split the replacement of .mobile into a separate ticket as there won't be any dependency.
Seems like given the broad use of wp_is_mobile() internal and by plugins it would make sense to split the tickets and then look at deprecating once all tickets are resolved?

#6 @johnbillion
14 months ago

Yep, splitting this out into separate issues would be a good way to tackled it.

#7 @johnbillion
14 months ago

  • Summary changed from Deprecate wp_is_mobile() to Reduce reliance on wp_is_mobile()

#8 @akibjorklund
14 months ago

33704-focus.diff handles autofocus-related usage of wp_is_mobile(). Turns out WordPress already has a mechanism for setting focus: you just add a class wp-initial-focus to an element. This was not used anywhere. So I just tweaked that to only fire when wpResponsive is not active. Plus, on install.php, where common.js is not available, tested against the viewport width instead.

Last edited 14 months ago by akibjorklund (previous) (diff)

#9 @akibjorklund
14 months ago

  • Keywords needs-testing added

33704-login.diff removes wp_is_mobile() from the login page.

The viewport meta tag is returned for all browsers as it has no effect on desktop browsers. I would have removed the whole function that does add it and just inlined the tag with the rest of head HTML, but I was not sure what the policy with removing functions is.

Secondly, no mobile class will be printed. There were three CSS rules in login.css that relied on that. The first one that moves the login box close to top I moved under a media query that test against the height of the viewport. The second one did set the left margin of the form to zero, but that was in any case zero, so the rule was redundant. The role of the third rule was mystery to me, it did just misalign links below the form with some extra left margin and thus did not seem useful to me and I removed it.

Lastly, the login shake animation is used for all browsers. I do not have any old mobile phones to test this with (as refresh rate with BrowserStack is poor and thus results inconclusive), so I cannot say if this is a poor experience. My gut tells me it won't perform very well but I doubt it is really an issue either. If it is, maybe a width based rule would do the trick here. I would appreciate if someone could test the patch with a phone with low processing power.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


13 months ago

#11 @desrosj
13 months ago

  • Keywords has-patch added; needs-patch removed

@akibjorklund has done a great job summarizing this. As noted by @curdin and @johnbillion, we should break this down into separate tickets. The initial patch provided needs some testing.

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


13 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


13 months ago

#14 @helen
13 months ago

  • Keywords needs-testing removed

Agree that this should probably be broken out into discrete tasks. I like the login one - there may be cases where people are assuming there will be a .mobile class and are using it but I'm okay with breaking that for now (can always put it back if we absolutely have to) and it can easily be added back via filter anyway.

The focus one is a little stranger to me in terms of what desired behavior truly is without considering what it currently does. I'm not sure why the width of a screen should be detected in JS in order to affect where initial focus is put, other than to recreate current functionality.

#15 @helen
13 months ago

In 38739:

Login: Don't rely on wp_is_mobile() for functionality.

Making behavior changes based on some broad definition of what mobile is rarely, if ever, makes sense. Each bit of functionality should be more clearly targeted, whether that's for screen size, performance, or some kind of touch capability.

props akibjorklund.
see #33704.

#16 @akibjorklund
13 months ago

@helen: The focus patch replicates current behaviour, yes. The reasoning behind that functionality is still valid. Mobile browsers don't do well with autofocus, because once a form element has focus, virtual keyboard pops up and it is hard to see anything else as it consumes a big portion of the viewport. It is useful when users knows what they are doing, but new users have hard time scrolling around and get a feel of the whole page/form.

I also looked into using the autofocus attribute, but the mobile browser implementation of that is spotty. Even if it worked, it would have the same issue mentioned above.

Autofocus is also problematic in terms of usability because it breaks the back button (backspace, cmd+left). As such, it should only be used when the purpose of the page is to fill in that field almost all of the time – think front page of Google.

Also, when I have observed people filling in forms with autofocus, they most of the time click the form element anyway to be sure there is focus – most people can't read the subtle language of focus.

Even though as a power user I would be unhappy if the feature would go away, I'm not sure very many people would even notice. Removal of the whole feature could thus also be considered.

#17 @adamsilverstein
13 months ago

related: in https://core.trac.wordpress.org/ticket/31652 my latest patch removes its use for jQuery UI Touch Punch which is causing problems.

#18 @adamsilverstein
12 months ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


12 months ago

This ticket was mentioned in Slack in #core by desrosj. View the logs.


12 months ago

#21 @adamsilverstein
12 months ago

  • Milestone changed from 4.7 to Future Release

Moving this out of the 4.7 milestone for now as we close in on beta. We have made some progress here, lets continue work on this in 4.9.

Last edited 5 months ago by adamsilverstein (previous) (diff)

#23 @adamsilverstein
5 months ago

  • Milestone changed from Future Release to 4.8.1

Will try to improve this for 4.8.1.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 months ago

#25 @jbpaul17
3 months ago

  • Milestone changed from 4.8.1 to 4.9

Punting to 4.9 per today's bug scrub.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 weeks ago

#27 @johnbillion
3 weeks ago

  • Keywords needs-testing added
  • Milestone changed from 4.9 to Future Release
Note: See TracTickets for help on using tickets.