Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The option <var name="allowfullscreen">false</var> for mobile device don't work in product view page gallery #12285

Closed
novakivskiy opened this issue Nov 15, 2017 · 15 comments
Assignees
Labels
Fixed in 2.2.x The issue has been fixed in 2.2 release line Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release

Comments

@novakivskiy
Copy link
Contributor

novakivskiy commented Nov 15, 2017

Preconditions

  1. Magento 2.1.9

Steps to reproduce

  1. edit file <theme>/etc/view.xml
  2. add <var name="allowfullscreen">false</var> to mobile breakpoints
    Example:
   <var name="breakpoints">
            <var name="mobile">
                <var name="conditions">
                    <var name="max-width">767px</var>
                </var>
                <var name="options">
                    <var name="options">
                        <var name="navigation">dots</var>
                        <var name="allowfullscreen">false</var>
                    </var>
                </var>
            </var>
        </var>
  1. clean cache and open source html
  2. find plugin "mage/gallery/gallery" init options on html page. The reason is in the type of the option "allowfullscreen" value, false as string, instead boolean.

"breakpoints": {"mobile":{"conditions":{"max-width":"767px"},"options":{"options":{"navigation":"dots","allowfullscreen":"false"}}}} }

Expected result

For screens smaller than <720px, do not open the gallery full screen

Actual result

  1. Screen http://take.ms/CfOWC
@novakivskiy novakivskiy changed the title The option <var name="allowfullscreen">false</var> for mobile device does not work in product view page gallery The option <var name="allowfullscreen">false</var> for mobile device does not work in product view page gallery Nov 15, 2017
@novakivskiy novakivskiy changed the title The option <var name="allowfullscreen">false</var> for mobile device does not work in product view page gallery The option <var name="allowfullscreen">false</var> for mobile device don't work in product view page gallery Nov 15, 2017
@magento-engcom-team magento-engcom-team added Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed and removed Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed labels Nov 15, 2017
@magento-engcom-team
Copy link
Contributor

@novakivskiy, thank you for your report.
We were not able to reproduce this issue by following the steps you provided. Please provide more detailed steps to reproduce.

@KirillLitvinenko
Copy link

Can reproduce on magento 2.2 by following steps.

Steps are the same:

  1. in <theme>/etc/view.xml in <var name="breakpoints"> option <var name="allowfullscreen"> to false

Code after changes:

<var name="breakpoints">
    <var name="mobile">
        <var name="conditions">
            <var name="max-width">767px</var>
         </var>
         <var name="options">
             <var name="options">
                 <var name="nav">dots</var>
                 <var name="allowfullscreen">false</var>
             </var>
         </var>
    </var>
</var>
  1. Clear cache

Expected result:

fullscrreen is disabled on mobile screens

Actual result:

fullscreen is working

Can be because of json that set to window object contains only string as value for breakpoints (example: "allowfullscreen": "false") object (should be bool for allowfullscreen key)

"breakpoints": {
  "mobile": {
    "conditions": {
      "max-width": "767px"
    }, 
    "options": {
      "options": {
        "nav": "dots", 
        "allowfullscreen": "false"
      }
    }
  }
}

@joebordo
Copy link

Just Posted the Same issue my self on magento 2.2.1.

@magento-engcom-team magento-engcom-team added Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed and removed Progress: needs update labels Nov 30, 2017
@magento-engcom-team
Copy link
Contributor

@novakivskiy, thank you for your report.
We've created internal ticket(s) MAGETWO-84793 to track progress on the issue.

@magento-engcom-team magento-engcom-team added 2.1.x Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Nov 30, 2017
@joebordo
Copy link

joebordo commented Nov 30, 2017

@KirillLitvinenko

You are correct. As a temporary solution i hard coded it in

module-catalog/view/frontend/templates/product/view/gallery.phtml
(in my theme)

I replaced

"breakpoints": <?= /* @escapeNotVerified */ $block->getBreakpoints() ?>

with

"breakpoints": {"mobile":{"conditions":{"max-width":"767px"},"options":{"options":{"nav":"dots","allowfullscreen":false}}}} }

Now it works as it should.

false was getting rendered as a string.

Thanks, your post really helped me!

cheers

@novakivskiy
Copy link
Contributor Author

novakivskiy commented Nov 30, 2017

As a solution, create a plugin for the class \Magento\Catalog\Block\Product\View\Gallery

    public function aroundGetBreakpoints(
        \Magento\Catalog\Block\Product\View\Gallery $subject,
        \Closure $proceed
    )
    {
        $json=  $proceed();
        $json = str_replace(['"true"','"false"'],['true','false'],$json);
        return $json;
    }

Or write a recursive function to bypass all nodes in the array. Where will the conversion be done from the string type to the type of booleans("true"->true)

@p-bystritsky p-bystritsky self-assigned this Dec 12, 2017
@okorshenko
Copy link
Contributor

Hi @novakivskiy
The issue has been fixed and delivered in 2.2-develop branch. Will be available with upcoming patch release.

@okorshenko okorshenko added the Fixed in 2.2.x The issue has been fixed in 2.2 release line label Dec 13, 2017
@novakivskiy
Copy link
Contributor Author

cool, thanks

@magento-engcom-team
Copy link
Contributor

Hi @novakivskiy. Thank you for your report.
The issue has been fixed in #15022 by @gwharton in 2.3-develop branch
Related commit(s):

The fix will be available with the upcoming 2.3.0 release.

@magento-engcom-team magento-engcom-team added the Fixed in 2.3.x The issue has been fixed in 2.3 release line label May 9, 2018
@magento-engcom-team
Copy link
Contributor

Hi @novakivskiy. Thank you for your report.
The issue has been fixed in #15021 by @gwharton in 2.3-develop branch
Related commit(s):

The fix will be available with the upcoming 2.3.0 release.

magento-engcom-team added a commit that referenced this issue May 9, 2018
…ar name='allowfullscreen'>false</var> for mobile device don't work in product view page gallery #15021

 - Merge Pull Request #15021 from gwharton/magento2:2.3-develop-12285
 - Merged commits:
   1. 435b165
   2. 4113f26
   3. c0b9e8f
   4. 05b1512
   5. 2701a4f
magento-engcom-team pushed a commit that referenced this issue May 9, 2018
…ar name="allowfullscreen">false</var> for mobile device don't work in product view page gallery #15021
@magento-engcom-team
Copy link
Contributor

Hi @novakivskiy. Thank you for your report.
The issue has been fixed in #15020 by @gwharton in 2.2-develop branch
Related commit(s):

The fix will be available with the upcoming 2.2.5 release.

@tmotyl
Copy link
Contributor

tmotyl commented Sep 4, 2018

2 days later the commit 8ef039e was reverted
in branch 2.2-develop without an explanation by @gwharton :
ca3245f#diff-49a84585354f5715382cc2b014a3d961

So the issue is still valid and gallery doesn't work.
Please also keep in mind that according to #12639 annotation @escapeNotVerified should NOT be used.

@gwharton
Copy link
Contributor

gwharton commented Sep 4, 2018

Yes, that was my fault. I have no idea how both I didnt see it, and the reviewers didnt see it, but yes it was accidentally reverted in a commit for another issue.

The revertion was corrected in PR #16594 in August though and should be available in 2.2.7.

@tmotyl
Copy link
Contributor

tmotyl commented Sep 4, 2018

@gwharton I've just seen your revert of the revert, but for some reason this change is not reflected in the file content, and the file history:
see https://github.com/magento/magento2/blob/2.2-develop/app/code/Magento/Catalog/view/frontend/templates/product/view/gallery.phtml

@gwharton
Copy link
Contributor

gwharton commented Sep 4, 2018

@tmotyl If you have time, can you take a look at the above PR and comment on changes.

ngantrant pushed a commit to ngantrant/magento-2-open-source that referenced this issue May 18, 2024
…">false</var> for mobile device don't work in product view page gallery"

This reverts commit b8232c5.
ngantrant pushed a commit to ngantrant/magento-2-open-source that referenced this issue May 18, 2024
…">false</var> for mobile device don't work in product view page gallery"

This reverts commit bf40afa.
ngantrant pushed a commit to ngantrant/magento-2-open-source that referenced this issue May 18, 2024
…">false</var> for mobile device don't work in product view page gallery"

This reverts commit ccc35c2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed in 2.2.x The issue has been fixed in 2.2 release line Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

No branches or pull requests

8 participants