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

Fixes #2576 by saving all attributes on the img tag. #2773

Merged
merged 7 commits into from Sep 11, 2015

Conversation

TimDix
Copy link
Contributor

@TimDix TimDix commented Jul 21, 2015

No description provided.

}

if (preg_match($imgmatch, $img->src, $matches)) {
$img->outertext = '<concrete-picture fID="' . $matches[1] . '" ' . $attrString . ' />';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that, for an empty attrString, this results in two consecutive spaces. This is why the Travis build is failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, tweaking now.

@TimDix
Copy link
Contributor Author

TimDix commented Jul 23, 2015

Alright, this latest 'fail' I think is the result of a poorly written test. The test asserts that

array('Test<img src="http://www.dummyco.com/path/to/server/index.php/download_file/view_inline/1">', 'Test<concrete-picture fID="1" alt="" style="" />'),

This is failing because the new code doesn't arbitrarily add empty attributes. I'd propose changing to to:

array('Test<img src="http://www.dummyco.com/path/to/server/index.php/download_file/view_inline/1">', 'Test<concrete-picture fID="1" />'),
array('Test<img src="http://www.dummyco.com/path/to/server/index.php/download_file/view_inline/1" alt="Woohoo" style="display: block" />', 'Test<concrete-picture fID="1" alt="Woohoo" style="display: block" />'),

@TimDix
Copy link
Contributor Author

TimDix commented Jul 23, 2015

Alternatively, if there's a good reason why the empty attributes should be there, I can force those to always output.

@MrKarlDilkington
Copy link
Contributor

👍

I tested the code and it applies the attributes to the img tag on save as expected.

Regarding any confusion relating to the placement of attributes.

http://www.w3.org/html/wg/drafts/html/master/semantics.html#the-picture-element

"While all of them contain source elements, the sourceelement's src attribute has no meaning when the element is nested within a picture element, and the resource selection algorithm is different. As well, thepicture element itself does not display anything; it merely provides a context for its contained img element that enables it to choose from multiple URLs"

@aembler
Copy link
Member

aembler commented Jul 27, 2015

No, there's no need to output the empty attributes. We probably just need to update the test so that it passes and then we'll get the pull request applied.

@MrKarlDilkington
Copy link
Contributor

@aembler

Will this fix be part of the next release?

aembler added a commit that referenced this pull request Sep 11, 2015
Fixes #2576 by saving all attributes on the img tag.
@aembler aembler merged commit deece36 into concretecms:develop Sep 11, 2015
@aembler
Copy link
Member

aembler commented Sep 11, 2015

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants