webcompat / web-bugs

A place to report bugs on websites.
https://webcompat.com
Mozilla Public License 2.0
740 stars 63 forks source link

www.sapporobeer.jp - image carousel not shown correctly because of old CSS #941

Closed karlcow closed 8 years ago

karlcow commented 9 years ago

URL: http://www.sapporobeer.jp/ Browser / Version: Firefox 39.0 Operating System: Android Problem type: Layout is messed up

Steps to Reproduce 1) Navigate to: http://www.sapporobeer.jp/ 2) …

Expected Behavior: Having a normal mobile layout (caroussel, etc.)

Actual Behavior: The caroussel is spread on multiple lines.

There are issues with android version number but this will not fixed the CSS issues.

sp.ua = function(){

    var o = {};

    var ua = navigator.userAgent;
    $.each(['iPhone', 'iPod', 'iPad'], function(i, current){
        o[current] = Boolean(ua.match(new RegExp(current, 'i')));
        o.appleDevice = o.appleDevice || o[current];
    });

    if (ua.indexOf('Android') !== -1) {
        o.Android = true;
        o.version = parseFloat(ua.match(/Android ([\d\.]+)(.*);/i)[1], 10);
        o.mobile = ua.match(/Mobile/i);
    }
    o.isSupport = o.iPhone || o.iPod || (o.Android && o.mobile && o.version >= 2.2);

    return o;

}();

The CSS issues

The caroussel

the carousel is at div.sp-carouselA01:nth-child(1) They use old antiquated Webkit css for flexbox.

.sp-carouselA01 .items,
.sp-carouselA01 .slide-unit-group,
.sp-carouselA01 .column-group {
    position: relative;
    display: -webkit-box;
    margin: 10px 0;
}

missing the display:flex; and a couple more.

Once these are fixed the site seem to be working as expected.

Buttons

some issues with some buttons, example with the usage of old gradients.

    .sp-buttonB01{
        font-weight:bold;
        display:inline-block;
        -webkit-border-radius:2px;
        border-radius:2px;
        border:1px solid #e2dbbf;
        background: #FFFFFF; /* old browsers */
        background: -moz-linear-gradient(top, #FFFFFF 0%, #EDEAE0 100%); /* firefox */
        background: -webkit-gradient(linear, left top, left bottom, color-stop(0%,#FFFFFF), color-stop(100%,#EDEAE0)); /* webkit */
        margin: 0 auto 10px;
        text-align:center;
        padding:.25em 0;
        -webkit-tap-highlight-color:transparent;
        cursor:pointer;
        width: 128px;
    }

country: jp

karlcow commented 9 years ago

Firefox unprefixed build

saporrobeer-unprefixed

Firefox Aurora

sapporobeer-aurora

miketaylr commented 9 years ago

@hallvors can you also look into the touch interactions on this site?

karlcow commented 9 years ago

There is a call center for Sapporo Beer which is also advertised in their twitter account.

Maybe a long shot but there is this Web agency @Moosylvania which worked on Sapporo campaign in USA. Maybe they can help us reach the right person in Sapporo USA and then from there reach Sapporo Japan. I wonder if @obj63mc can help.

obj63mc commented 9 years ago

Hi @karlcow - not sure who can help with sapporobeer.jp but looking at the css above in general there are no 'un-prefixed' methods of the css so you have -webkit- and -moz- but none with out the prefix. Firefox as started to not require prefixing for a lot of standard css3 functionality like gradients, border radius, drop shadow etc.

I'll forward this on to our contact at Sapporo USA though to see if they can bring this to whomever's attention.

karlcow commented 9 years ago

@obj63mc the unprefixed build is a build of Firefox for Android which fixes gradients and flex issues and a couple of others. It basically converts the CSS rules. It's maybe what have might led you to think about just unprefixing. Sorry about that it was a bit confusing. Full disclosure: I'm working for Mozilla. ;)

Thanks a lot for helping us to reach the right person. Really appreciated.

obj63mc commented 9 years ago

Thanks @karlcow - totally makes sense. I have forward this on and if I hear anything I'll be sure to update this issue.

hallvors commented 9 years ago

@miketaylr not sure what touch actions you want me to look at - seems the carousel is the only problem

miketaylr commented 9 years ago

@hallvors below the top carousel, you should be able to flick through the two rows of content (the first one looks like plates/recipes). It doesn't work for me in Firefox for Android.

hallvors commented 9 years ago

All "carousel"-like parts of the page seems based on the same code. First, as @karlcow says the display:-webkit-box here needs a display:flex to go with it:

.sp-carouselA01 .items, .sp-carouselA01 .slide-unit-group, .sp-carouselA01 .column-group {
  position: relative;
  display: -webkit-box;
  margin: 10px 0;
}

That sorts out the layout of the carousel parts so they are no longer stacked vertically. Secondly, this JS

        self.element.style.webkitTransitionProperty = '-webkit-transform';
        self.element.style.webkitTransitionTimingFunction = 'cubic-bezier(0,0,0.25,1)';
        self.element.style.webkitTransitionDuration = '0';
        self.element.style.webkitTransform = getTranslate(0);

and similar code in flipsnap.js needs to handle non-webkit-prefixed stuff too. Finally the code wants an Android version number - if the above and the ua.match() Karl mentions is fixed, the sliding/dragging should work.

miketaylr commented 9 years ago

@hallvors What confuses me is that testing with an Android version number and a "whitelisted" build does not result in a working site. Did you get different results?

karlcow commented 9 years ago

@miketaylr not working how. I got a working site myself.

kudodo commented 9 years ago

@karlcow My Firefox 39 gets broken layout and Nightly(41, 42) get layout fixed but not functional. What kind of information should I share with Sapporo?

karlcow commented 9 years ago

@kudodo still digging to understand.

@miketaylr yup I tested on Thursday. And the site is not working for me either. It is somehow like some touch events are busted. Arrows on the carrousel, sliding the photos

http://www.sapporobeer.jp/js/shared/flipsnap.js

    moveToPoint: function(point) {
        var self = this;

        self.currentPoint = 
            (point < 0) ? 0 :
            (point > self.maxPoint) ? self.maxPoint :
            parseInt(point);

        self.element.style.webkitTransitionDuration = '350ms';
        self._setX(- self.currentPoint * self.distance)

        var ev = document.createEvent('Event');
        ev.initEvent('flipsnap.moveend', true, false);
        self.element.dispatchEvent(ev);
    },
    _setX: function(x) {
        var self = this;

        self.currentX = x;
        self.element.style.webkitTransform = getTranslate(x);
    },

If I check the value of self.element.style.webkitTransform = getTranslate(x); in the process of sliding, I get webkitTransform: "translate(-909.8823529411766px, 0)" when transitioning from 3rd to 4th slide.

capture d ecran 2015-07-13 a 12 12 57

which seems reasonable but it doesn't get applied in the end.

karlcow commented 9 years ago

Oh wait… Flipsnap seems to have been updated to support more than WebKit browsers. https://github.com/hokaccha/js-flipsnap/

There are using @version 0.1.3. The current flipsnap is at @version 0.6.2 Maybe the simpler thing to do would be for them to update to the last version of flipsnap.

hallvors commented 9 years ago

I confirm that the latest flipsnap version works fine if the UA has an Android version number (or they fix their sniffing.

There's one more error message they may want to fix, although I don't know if it breaks anything. In http://www.sapporobeer.jp/js/shared/smartphone.top.js the function protectImg() is defined inside an if(){} block. It's not a good idea to put function declarations inside conditional blocks because you trigger browser incompatibilities. Move it outside of the if - or move the line that calls protectImg() below the code that defines the function.

hallvors commented 9 years ago

(Trying to fix all the webkitTransition and webkitTransform usage in their current flipsnap library fixed all sliding rows of pictures except the top one. If the website is not interested in updating the lib, we have to make sure unprefixing handles webkitTransition* and webkitTransform - and debug the last issue with the top slideshow.)

miketaylr commented 9 years ago

I confirm that the latest flipsnap version works fine if the UA has an Android version number

We have that in the UA from version 41+, so that's good.

karlcow commented 9 years ago

@obj63mc did you have any success with Sapporo Beer USA? @kudodo Do you know if we have contacts with Sapporo Beer Japan?

Now that we know exactly what needs to be done.

kudodo commented 9 years ago

I'll try to contact from their web site in Japan.

karlcow commented 9 years ago

Looking a bit more at the device detection. sp.ua returns an object, for example, when accessing with Firefox OS, we get. I'm pretty sure something similar for Microsoft Mobile. ( @theWebjustworks )

Object { 
   iPhone: false, 
   appleDevice: false, 
   iPod: false, 
   iPad: false, 
   isSupport: undefined }

if I access with Firefox Android with version number I get

Object { 
   iPhone: false, 
   appleDevice: false, 
   iPod: false, 
   iPad: false, 
   Android: true, 
   version: 4.4, 
   mobile: Array[1], 
   isSupport: true }

The mobile array has "Mobile" and

sp.ua.mobile[0]
"Mobile"
sp.ua.mobile.index
28
sp.ua.mobile.input
"Mozilla/5.0 (Android 4.4.4; Mobile; rv:38.0) Gecko/38.0 Firefox/38.0"

sp.ua is used twice only. First in this piece of code only for iOS devices

if (sp.ua.iPhone || sp.ua.iPod) {
    $('.sp-mod-appinfo01').show();
}

then in

sp.modeManager = (function(){
/* cut code */
            }else if(sp.ua.isSupport) {
                if (isTop && location.search === '?mode=pc') {
                    return 'forcePc';
                }
                else {
                    return 'sp';
                }

            }else{
/* cut code */
    })();

The sp.modeManager is used for affecting changes in between pc and smartphones including a debug mode. It adjusts the CSS, the meta viewport, etc. It doesn't seem to trigger anything else which would be harmful to other mobile browsers. If @miketaylr sees anything.

To note that there is another UA detection, to protect image from being copied?

var uA = navigator.userAgent;
if ( uA.indexOf('Android') > 0 || uA.indexOf('iPhone') > 0 || uA.indexOf('iPad') > 0 ) {
    protectImg( 'body.home .sp-carouselA01 .figure a img' );

Above for example with an ipod you don't get the protectImg. ^_^

All of that to say that they could simplify a lot everything. I guess they could reuse sp.ua for the protectImg, that would make it a bit more dry. AND sp.ua could be simplified.

sp.ua = function(){
…
    o.isSupport = o.iPhone || o.iPod || (o.Android && o.mobile && o.version >= 2.2);
…
}();

so the requirement on the version is dropped. (not sure what is the reason for 2.2). iPhone, iPod, iPad have the "Mobile" keyword.

So you could set o.isSupport when the UA

karlcow commented 8 years ago

fixed by layout.css.prefixes.webkit;true