Joomla! Issue Tracker | Joomla! CMS #30790 - [4.0] loading=lazy HTMLHelper

? ?

Fixed in Code Base
27 Oct 2020
Medium
Build: 4.0-dev
# 30790
Diff
zero-24:lazyload_htmlhelper

Pending

Success continuous-integration/appveyor/pr AppVeyor build succeeded Details
Success Download Prebuilt packages are available for download. Details
Success continuous-integration/drone/pr Build is passing Details
Pending JTracker/HumanTestResults Human Test Results: 2 Successful 1 Failed. Details

User tests: Successful: Unsuccessful:

zero-24
27 Sep 2020

Pull Request for Issue #30615

Summary of Changes

Make sure we support both strings and arrays passed as attributes. cc @SharkyKZ

Testing Instructions

install 4.0-dev
setup an contact with an contact image
add it to a menu
open it on the frontend
check that the loading=lazy attribute is there.
Add <?php echo Joomla\CMS\HTML\HTMLHelper::_('image', 'mod_languages/de.gif', '', null, true); ?> to this line https://github.com/joomla/joomla-cms/blob/4.0-dev/templates/cassiopeia/index.php#L106
Notice the german flag when checking the frontend.
notice it does not has the loading attribute set.
apply the patch
notice that loading=lazy is gone for the contact menu image too.
notice there is no waring when using the code mention above anymore.

Actual result BEFORE applying this Pull Request

warning when using NULL or an string as attributes

Expected result AFTER applying this Pull Request

no warning anymore and no default to loading=lazy

Documentation Changes Required

None

2e2a577 27 Sep 2020

Fix HTML Helper Lazyloading

969faae 27 Sep 2020

add missing ;

c502675 27 Sep 2020

also check for NULL

zero-24 - open - 27 Sep 2020

zero-24 - change - 27 Sep 2020

Status

New

⇒

Pending

joomla-cms-bot - change - 27 Sep 2020

Category

⇒

Libraries

9b19e3f 27 Sep 2020

Update libraries/src/HTML/HTMLHelper.php

zero-24 - change - 27 Sep 2020

Labels

Added: ?

ceford - comment - 28 Sep 2020

I found that I needed to add use Joomla\CMS\HTML\HTMLHelper; to index.php and the German flag has loading="lazy" whether or not the patch is applied. Does this mean the patch can only be tested with a new install?

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

zero-24 - comment - 28 Sep 2020

Yes sorry that use was missing from my test instructions.

SharkyKZ - comment - 28 Sep 2020

Running unnecessary type checks. Running strpos() against null. Attribute is added when already exists. Attribute not added when another attribute containing loading in its name exists. Does not account for whitespaces.

adj9 - test_item - 28 Sep 2020 - Not tested

adj9 - comment - 28 Sep 2020

I have not tested this item.

With step 6 I logged into the frontend and on each page I have the message:

The requested page can't be found.

With and without PR.

If you restore the index.php file, I don't have the message.

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

adj9 - test_item - 28 Sep 2020 - Not tested

adj9 - comment - 28 Sep 2020

I have not tested this item.

with use Joomla\CMS\HTML\HTMLHelper; done PR

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

adj9 - comment - 28 Sep 2020

I have not tested this item.

with use Joomla\CMS\HTML\HTMLHelper; done PR

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

adj9 - test_item - 28 Sep 2020 - Tested successfully

adj9 - comment - 28 Sep 2020

I have tested this item ✅ successfully on 9b19e3f

with use Joomla\CMS\HTML\HTMLHelper; done PR

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

zero-24 - change - 28 Sep 2020

The description was changed

zero-24 - edited - 28 Sep 2020

513c407 28 Sep 2020

use elseif and make an is null condition

zero-24 - comment - 28 Sep 2020

Running unnecessary type checks. Running strpos() against null. Attribute is added when already exists. Attribute not added when another attribute containing loading in its name exists. Does not account for whitespaces.

Thanks should be fixed now.

with use Joomla\CMS\HTML\HTMLHelper; done PR

Yes sorry updated the description too :)

fce82b1 28 Sep 2020

Update libraries/src/HTML/HTMLHelper.php

SharkyKZ - comment - 28 Sep 2020

The check for existing attribute is still wrong.

zero-24 - comment - 28 Sep 2020

The check for existing attribute is still wrong.

What is wrong do you have a bit more detail? How can we do it better?

dgrammatiko - comment - 28 Sep 2020

@zero-24 since my other PR #30784 is defaulting but not forcing only lazyloaded images I think these changes need some rethinking. I mean the helper already has everything it needs, therefore there is no need for special treatment for lazyloading, eg: https://repl.it/@dgrammatiko/HTMLHelperImage#main.php

I guess we overdid it when we introduced lazyloading...

My 2c anyways

zero-24 - comment - 29 Sep 2020

since my other PR #30784 is defaulting but not forcing only lazyloaded images I think these changes need some rethinking.

hmm it is the same here. It is just defaulting but not forcing only. As you can pass loading=eager and than we dont add loading=lazy. The reason is that when you use the html helper the developer knows whether that can / should be lazyloaded or not so they can configure the html helper.

dgrammatiko - comment - 29 Sep 2020

hmm it is the same here. It is just defaulting but not forcing only.

I guess then the code part that checks for the string attributes should be changed to reflect that, eg if there is no loading=eager or loading = auto then add lading = lazy, so it needs a bit of regex...

SharkyKZ - comment - 29 Sep 2020

It is just defaulting but not forcing only.

But it shouldn't. Developers should really pass this on their own, preferably with image dimensions. Such a generic method just shouldn't be stuffing random attributes nobody asks for.

dgrammatiko - comment - 29 Sep 2020

Developers should really pass this on their own,

@SharkyKZ I will agree, maybe expand the docs block with an example is all we need here, eg: https://repl.it/@dgrammatiko/HTMLHelperImage#main.php

a642024 29 Sep 2020

drop auto lazyloading images

zero-24 - comment - 29 Sep 2020

Done

zero-24 - comment - 29 Sep 2020

(i dont see any extension dev taking up on that but lets see how it evolves)

zero-24 - change - 4 Oct 2020

The description was changed

zero-24 - edited - 4 Oct 2020

zero-24 - comment - 4 Oct 2020

Test instructions has been updated as requested right now the default has been fully removed with the developer to choose and use the attribute that is already there.

zero-24 - change - 4 Oct 2020

The description was changed

zero-24 - edited - 4 Oct 2020

tushar33 - test_item - 17 Oct 2020 - Tested unsuccessfully

tushar33 - comment - 17 Oct 2020

I have tested this item ? unsuccessfully on a642024

1. Image is broken without applying patch and after
2. no warning when using NULL or an string as attributes

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

zero-24 - comment - 17 Oct 2020

@tushar33

Image is broken without applying patch and after

Than this is not an issue with this specific PR and should get its own issue. Can you please post the steps you took and make a new Issue so we keep track of that?

PhocaCz - test_item - 17 Oct 2020 - Tested unsuccessfully

PhocaCz - comment - 17 Oct 2020

I have tested this item ? unsuccessfully on a642024

Hi, I get no warning when using NULL or an string as attribute (tested J! 4 Beta 5, tested prebuilt package)

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

dgrammatiko - test_item - 17 Oct 2020 - Tested successfully

dgrammatiko - comment - 17 Oct 2020

I have tested this item ✅ successfully on a642024

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

dgrammatiko - comment - 17 Oct 2020

Hi, I get no warning when using NULL or an string as attribute (tested J! 4 Beta 5, tested prebuilt package)

What kind of warning do you expect?
This exact code exists in Joomla for many years, all this PR is doing is reverting the enforced addition of one single attribute...

richard67 - comment - 17 Oct 2020

@tushar33 @PhocaCz Please reset your test result to "I have not tested this item" (or so) in the issue tracker if your issue is not related to this PR (what I assume now after reading). If necessary open a new issue. Tanks in advance.

PhocaCz - test_item - 17 Oct 2020 - Not tested

PhocaCz - comment - 17 Oct 2020

I have not tested this item.

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

Quy - test_item - 19 Oct 2020 - Tested successfully

Quy - comment - 19 Oct 2020

I have tested this item ✅ successfully on a642024

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

Quy - change - 19 Oct 2020

Status

Pending

⇒

Ready to Commit

Quy - comment - 19 Oct 2020

RTC

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

rdeutz - close - 27 Oct 2020

rdeutz - merge - 27 Oct 2020

rdeutz - change - 27 Oct 2020

Status	Ready to Commit	⇒	Fixed in Code Base
Closed_Date	0000-00-00 00:00:00	⇒	2020-10-27 12:46:38
Closed_By		⇒	rdeutz

Add a Comment

Older
Newer

Joomla! Issue Tracker - CMS

[#30790] - [4.0] loading=lazy HTMLHelper

Summary of Changes

Testing Instructions

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

Add a Comment