improve mobile checks #15228

photodude · 2017-04-11T15:59:39Z

Pull Request for Issue mobile checks have multiple conditionals when one is sufficient

Summary of Changes

only check for resolution in user agent once to determine mobile
add common user agent strings that identify mobile
Fix mobile check to be separate from browser.

Testing Instructions

code review

(could apply patch and check on a mobile device or emulator that mobile is detected)

Expected result

mobile is easily and correctly detected.

Actual result

extraneous checks were in additional locations specific to certain browsers

Documentation Changes Required

none

only check for resolution in user agent once to determine mobile

photodude · 2017-04-22T15:50:50Z

@rdeutz, @mbabker, @wilsonge
Any thoughts on these changes?

wilsonge · 2017-04-22T16:28:38Z

I think this is wrong. For example edge handsets will now just be recognised as mobile but not as edge. Because they will trigger in the first if that does the mobile check, but not in the part that does the

Ditto I assume iPhone's can be identified as Safari (or iOS Chrome by the user agent) and therefore shouldn't 'just' be a unknown browser mobile. But should trigger user agent checks so we know mobile and browser

But I am not a user agent guru at all :)

dgrammatiko · 2017-04-22T16:39:11Z

I've proposed this before, but I will do it again, let's use: https://github.com/serbanghita/Mobile-Detect

brianteeman · 2017-04-22T17:07:18Z

Dont we have mobile detection already in https://github.com/joomla/joomla-cms/blob/staging/libraries/vendor/joomla/application/src/Web/WebClient.php

photodude · 2017-04-22T17:07:28Z

@wilsonge I believe mobile is a wholly separate property from browser. I also believe the two are checked and set separately (although in the same function).

dgrammatiko · 2017-04-22T17:22:23Z

@brianteeman and this is one of those areas that Joomla should not try to innovate by having it's own well abandoned code. Check the highlighted part of their repo and compare it to the last update of the Joomla's shiny WebClient.php:

Use other people's code when it's an obvious win!

mbabker · 2017-04-22T17:25:37Z

If someone does a PR that at least proxies the internals of either JBrowser or Joomla\Application\Web\WebClient (or both) to another library we could use it. We'd need that to help with the transition away from our own code.

wilsonge · 2017-04-22T18:11:52Z

@photodude if you match this if https://github.com/photodude/joomla-cms/blob/f1a5b8df900c31d21e93291286dedd405ca5320c/libraries/joomla/environment/browser.php#L229-L242 then you're never going to reach the edge browser elseif afterwards

photodude · 2017-04-22T18:47:47Z

@wilsonge that is technically a bug in the existing code too I overlooked the if elseif; I was thinking it was two different conditionals as it should be. Corrected, as mobile and browser are different properties.

photodude · 2017-04-22T19:00:01Z

@dgt41 I'm all for using an external solution. We will never be able to properly maintain every browser combination and properly check them.
See joomla-framework/application#59 where we have been discussing that for Joomla\Application\Web\WebClient

but as @mbabker noted, someone needs to proxy the internals of our existing solutions to whatever option we use. I'm leaning towards pulling in the browscap.ini and use the php native get_browser() although that requires setting another ini value at runtime with ini_set('browscap', '[thebrowscap.ini file location]'); and parsing the returned object. I'm leaning that direction since many other userland browscap libraries don't support the range of PHP versions that Joomla does.

photodude · 2017-04-22T19:05:55Z

@brianteeman Yes, there is a mobile detection in Joomla\Application\Web\WebClient. as there currently is here in JBrowser. There is work needed here; either fixing, or proxying to a 3rdparty library, or making JBrowser an alias of Joomla\Application\Web\WebClient

The focus in this PR is improving (and apparently fixing) the current JBrowser mobile detection.

photodude · 2017-04-22T19:10:08Z

@mbabker can you rerun the appveyor tests. There was an unrelated failure in one of the Cachelite tests. Thanks in advance.

photodude · 2017-04-29T16:00:57Z

@wilsonge with the correction to the conditional, do you think this is good to go?

Quy · 2017-05-30T18:12:33Z

libraries/joomla/environment/browser.php

 			if (strpos($this->lowerAgent, 'mobileexplorer') !== false
 				|| strpos($this->lowerAgent, 'openwave') !== false
 				|| strpos($this->lowerAgent, 'opera mini') !== false
 				|| strpos($this->lowerAgent, 'opera mobi') !== false
-				|| strpos($this->lowerAgent, 'operamini') !== false)
+				|| strpos($this->lowerAgent, 'operamini') !== false
+				|| preg_match('/(iPhone|iPod|iPad|Android|Mobile|Phone|BlackBerry)/i', $this->agent);


Blackberry is tested on line 413 so it can be removed here.

I would go the other way and remove the mobile set in the 413 conditional. mobile and browser should check/set separately.

If that is the case, then starting with line 378 to 413 would have to be changed.

I'll try to get to those on Wednesday evening

@Quy the changes have been implemented

infograf768 · 2017-05-31T07:13:14Z

Question:
Can we serve different contents depending on this detection?
For example, a different content depending if mobile or desktop.
And usable in a multilingual setting?

photodude · 2017-05-31T16:21:03Z

@infograf768 It might be possible. Although, I'm more of a fan of responsive design and let the css control the view depending on the viewport size. As for content, it seems reasonable that a module could be set to change content when JBrowser::mobile === true but I am unaware of any examples showing such a content switching based on the value of JBrowser::mobile.

I've done a few things in the past where a module would show if it was desktop and would hide and show a different module if it was mobile, that switch was all controlled by css classes.

Quy · 2017-06-01T15:21:44Z

libraries/joomla/environment/browser.php

+				|| preg_match('/(openwave|opera mini|opera mobi|operamini|avantgo|wap|elaine)/i', $this->agent)
+				|| preg_match('/(iPhone|iPod|iPad|Android|Mobile|Phone|BlackBerry|Xiino|Palmscape|palmsource)/i', $this->agent)
+				|| preg_match('/(Nokia|Ericsson|docomo|portalmmm|CriOS[/ ]([0-9.]+)|)/i', $this->agent)
+				|| preg_match('/(digital paths|UP|UP.B|UP.L)/i', $this->agent)


Warning: preg_match(): Unknown modifier ']' in C:\xampp\htdocs\joomla-cms\libraries\joomla\environment\browser.php on line 238 Warning: preg_match(): Unknown modifier 'C' in C:\xampp\htdocs\joomla-cms\libraries\joomla\environment\browser.php on line 280

For UP probably will have to append / to prevent false match since it is doing an incase-sensitive match.

@Quy I can not reproduce the Unknown modifier warnings. I have made some adjustments to the patterns that should fix those. Let me know if they are still occurring on your end.

Quy · 2017-06-01T19:28:39Z

libraries/joomla/environment/browser.php

-				|| strpos($this->lowerAgent, 'operamini') !== false)
+				|| strpos($this->agent, 'MOT-') !== false
+				|| strpos($this->lowerAgent, 'j-') !== false
+				|| preg_match('/(openwave|opera mini|opera mobi|operamini|avantgo|wap|elaine)/i', $this->agent)


How about combining mobileexplorer here?

Quy · 2017-06-01T22:16:45Z

I have tested this item ✅ successfully on 4328de0

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15228.}

ghost · 2017-06-02T07:12:31Z

Have tested successfully using "Firefox > Responsive Design Mode" – ist this Test-Scenario enough for marking as successfully Test?

photodude · 2017-06-02T14:02:21Z

@franz-wohlkoenig Looking over the code JBrowser seems to only be used in JHtml::includeRelativeFiles and affects JHtml::stylesheet. Any general testing should be sufficient unless you can find a template that uses different files for different browsers and want to test with multiple browsers. Since that is a very unique situation, general use testing, Code review and the unit tests should be sufficient for now.

Likely need to look at depreciating JBrowser for 4.0 and just use the framework browser detection in the application library.

ghost · 2017-06-02T14:05:37Z

I have tested this item ✅ successfully on 4328de0

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15228.}

ghost · 2017-06-02T14:06:09Z

RTC after two successful tests.

wilsonge · 2017-06-02T18:50:08Z

Thanks!

infograf768 · 2017-07-16T06:35:43Z

@photodude
Please look at #17139 urgently

photodude · 2017-07-16T19:28:57Z

@infograf768 I have reviewed it.

improve mobile checks

833f4e1

only check for resolution in user agent once to determine mobile

joomla-cms-bot added the PR-staging label Apr 11, 2017

photodude added 3 commits April 11, 2017 10:02

[cs] fix tabs vs spaces from copy paste

b6e2a6b

fix the copy-paste peak-a-boo semicolon

d691c72

Merge branch 'staging' into patch-6

f1a5b8d

zero-24 added this to the Joomla 3.7.1 milestone Apr 22, 2017

Mobile is different from browser. Fix conditionals

9de5db3

Merge branch 'staging' into patch-6

d20c5b8

zero-24 modified the milestones: Joomla 3.7.1, Joomla 3.7.2 May 12, 2017

rdeutz modified the milestones: Joomla 3.7.2, Joomla 3.7.3 May 18, 2017

Merge branch 'staging' into patch-6

6cdeb86

photodude added 2 commits May 29, 2017 19:55

Merge branch 'staging' into patch-6

ff13a53

Merge branch 'staging' into patch-6

c305767

Quy reviewed May 30, 2017

View reviewed changes

photodude added 3 commits June 1, 2017 06:07

Merge branch 'staging' into patch-6

42911f2

continue moving mobile checks to a separate conditional

ba523b6

Merge branch 'staging' into patch-6

e669ea9

Quy reviewed Jun 1, 2017

View reviewed changes

attempt fix for Unknown modifier on lines 238/280

486b0bf

Quy reviewed Jun 1, 2017

View reviewed changes

combining mobileexplorer

4328de0

joomla-cms-bot removed this from the Joomla 3.7.3 milestone Jun 2, 2017

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 2, 2017

wilsonge merged commit 3f15a2d into joomla:staging Jun 2, 2017

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 2, 2017

wilsonge modified the milestones: Joomla 4.0, Joomla 3.7.3 Jun 2, 2017

photodude deleted the patch-6 branch June 2, 2017 19:39

Quy mentioned this pull request Jul 6, 2017

Regression: Fix undefined offset: 1 #16989

Closed

Quy mentioned this pull request Jul 15, 2017

Regression: Fix undefined offset: 1 #17139

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve mobile checks #15228

improve mobile checks #15228

photodude commented Apr 11, 2017 •

edited by joomla-cms-bot

photodude commented Apr 22, 2017

wilsonge commented Apr 22, 2017 •

edited

dgrammatiko commented Apr 22, 2017

brianteeman commented Apr 22, 2017

photodude commented Apr 22, 2017

dgrammatiko commented Apr 22, 2017

mbabker commented Apr 22, 2017

wilsonge commented Apr 22, 2017 •

edited

photodude commented Apr 22, 2017

photodude commented Apr 22, 2017

photodude commented Apr 22, 2017

photodude commented Apr 22, 2017

photodude commented Apr 29, 2017

Quy May 30, 2017

photodude May 30, 2017

Quy May 30, 2017

photodude May 30, 2017

photodude Jun 1, 2017 •

edited

infograf768 commented May 31, 2017

photodude commented May 31, 2017

Quy Jun 1, 2017

photodude Jun 1, 2017

Quy Jun 1, 2017

Quy commented Jun 1, 2017

ghost commented Jun 2, 2017

photodude commented Jun 2, 2017

ghost commented Jun 2, 2017

ghost commented Jun 2, 2017

wilsonge commented Jun 2, 2017

infograf768 commented Jul 16, 2017

photodude commented Jul 16, 2017

improve mobile checks #15228

improve mobile checks #15228

Conversation

photodude commented Apr 11, 2017 • edited by joomla-cms-bot

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

photodude commented Apr 22, 2017

wilsonge commented Apr 22, 2017 • edited

dgrammatiko commented Apr 22, 2017

brianteeman commented Apr 22, 2017

photodude commented Apr 22, 2017

dgrammatiko commented Apr 22, 2017

mbabker commented Apr 22, 2017

wilsonge commented Apr 22, 2017 • edited

photodude commented Apr 22, 2017

photodude commented Apr 22, 2017

photodude commented Apr 22, 2017

photodude commented Apr 22, 2017

photodude commented Apr 29, 2017

Quy May 30, 2017

Choose a reason for hiding this comment

photodude May 30, 2017

Choose a reason for hiding this comment

Quy May 30, 2017

Choose a reason for hiding this comment

photodude May 30, 2017

Choose a reason for hiding this comment

photodude Jun 1, 2017 • edited

Choose a reason for hiding this comment

infograf768 commented May 31, 2017

photodude commented May 31, 2017

Quy Jun 1, 2017

Choose a reason for hiding this comment

photodude Jun 1, 2017

Choose a reason for hiding this comment

Quy Jun 1, 2017

Choose a reason for hiding this comment

Quy commented Jun 1, 2017

ghost commented Jun 2, 2017

photodude commented Jun 2, 2017

ghost commented Jun 2, 2017

ghost commented Jun 2, 2017

wilsonge commented Jun 2, 2017

infograf768 commented Jul 16, 2017

photodude commented Jul 16, 2017

photodude commented Apr 11, 2017 •

edited by joomla-cms-bot

wilsonge commented Apr 22, 2017 •

edited

wilsonge commented Apr 22, 2017 •

edited

photodude Jun 1, 2017 •

edited