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

Align Version button with Save / Cancel on front-end edit. #4369

Closed
wants to merge 2 commits into from
Closed

Align Version button with Save / Cancel on front-end edit. #4369

wants to merge 2 commits into from

Conversation

Sophist-UK
Copy link
Contributor

Fix for issue #4637

If you have versioning on, then a front end editing page has the version button misaligned with the Save and Cancel buttons.

This is due to the Save and Cancel buttons being rendered with the <button> tag whilst Version is rendered with the <a> tag - and different CSS is applied.

image

The fix is to change the <a> tag for a <button> tag in /libraries/cms/form/field/contenthistory.php at lines 49/53.

@zero-24
Copy link
Contributor

zero-24 commented Sep 27, 2014

@Sophist-UK hmm it looks strange but I need to mark a unsuccessfull test.

First i can't reproduce the issue with core protostar:
screen shot 2014-09-27 at 12 48 11

And after the patch we indroduce a new issue. If you click the button after the patch the modal don't open. It redirect me to http://localhost/git/index.php/component/content/?a_id=24 on my local test envoirment.

Can you have a look into it?

This comment was created with the J!Tracker Application at http://issues.joomla.org/.

@Sophist-UK
Copy link
Contributor Author

  1. Yes - I find the same. I will look into whether this can be solved.
  2. I have identified the cause and it is fixable - but I need a little further research to ensure that there are not unintended consequences.

@Sophist-UK
Copy link
Contributor Author

Fixed.

@zero-24
Copy link
Contributor

zero-24 commented Sep 28, 2014

thanks works now @Sophist-UK

This comment was created with the J!Tracker Application at http://issues.joomla.org/.

@dgrammatiko
Copy link
Contributor

@Sophist-UK Is rel attribute compatible with button? I mean the page still validates? http://validator.w3.org

@Sophist-UK
Copy link
Contributor Author

Yes - that may be an issue as it is not (AFAIK) officially part of the HTML spec. I have tested this in IE11 and Chrome and it works fine, but of course that doesn't guarantee it will work on other browsers or older versions of these browsers.

That said, the way that we use the rel attribute on the existing tag is not according to the HTML spec anyway (which states that rel needs to be one of "alternate, author, bookmark, help, license, next, nofollow, noreferrer, prefetch, prev, search, tag" - whilst we use it to hold a JSON for the iframe). In fact the href attribute is also not part of the spec - other buttons (like Save and Cancel) use an OnClick js method (e.g. Onclick="Joomla.submitbutton('article.save')") to action the button click whilst Version uses a different means of setting the click event using jQuery code created by JHtml::_('behavior.modal', 'button.modal_' . $this->id);).

So, AFAICT the neither the existing Version tag nor my proposed

alternative would validate correctly. But in practice, I think that most browsers will parse the href and rel attributes into the DOM ok, and the JS will set the same on-click event on an <a> or <button> object.

So, on the whole I think this should be OK.

This comment was created with the J!Tracker Application at http://issues.joomla.org/.

@zero-24
Copy link
Contributor

zero-24 commented Sep 28, 2014

http://validator.w3.org

@Sophist-UK the validator tell me a error with the href attribut.

 Line 362, Column 351: Attribute href not allowed on element button at this point.

…d0dbe297f0abda46cc522df9b=1" rel="{handler: 'iframe', size: {x: 800, y: 500}}">

Attributes for element button:
    Global attributes
    autofocus
    disabled
    form
    formaction
    formenctype
    formmethod
    formnovalidate
    formtarget
    menu
    name
    type
    value

This is line 362

<button class="btn modal_jform_contenthistory" title="Versions" href="/git/index.php/component/contenthistory/?view=history&amp;layout=modal&amp;tmpl=component&amp;field=jform_contenthistory&amp;item_id=24&amp;type_id=1&amp;type_alias=com_content.article&amp;af11649d0dbe297f0abda46cc522df9b=1" rel="{handler: 'iframe', size: {x: 800, y: 500}}">

@zero-24
Copy link
Contributor

zero-24 commented Sep 28, 2014

But it works for me on

  • Firefox 32.0.3
  • Opera 24.0.1558.64
  • Chrome 37.0.2062.124 m
  • IE 11 (11.0.960.17278; Updateversion: 11.0.12)

@Sophist-UK
Copy link
Contributor Author

Yes - I got the same validator error messages. But overall I still think it is OK.

@dgrammatiko
Copy link
Contributor

@Sophist-UK I didn’t want to sound negative but I also tried a few weeks before to tackle the same in a different way (wrapping an a tag around a button) as you can see here #4244. Just saying...

@Sophist-UK
Copy link
Contributor Author

@dgt41 - No offence taken I assure you. And providing the reference to your PR is a useful link.

@dgrammatiko
Copy link
Contributor

@Sophist-UK What I had in my mind, but never found the time to code it, was exactly what you did here but instead of using the JHtml::_('behavior.modal', 'button.modal_' . $this->id); copy the code from html.php and make the initialization of modal there so no href and rel attributes spilled inside the button...

@roland-d
Copy link
Contributor

roland-d commented Oct 2, 2014

@zero-24 & @Sophist-UK How do you reproduce the issue? Is the PR to fix the Versions button alignment or just to make it consistent using the tag?

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4369.

@Sophist-UK
Copy link
Contributor Author

I get this issue with the Shape Vertex template, but essentially it happens whenever the css for <a> and <button> have different top margins. So to recreate, you just need to have custom css which changes the top margin for tags.

The only way to ensure consistency across all templates is to use the same tag for all buttons. I would not have a problem converting <button> tags to <a> tags, but this is a larger changes, and the version functionality is newer and so it seemed to me that we should change this rather than the other buttons.

@roland-d
Copy link
Contributor

roland-d commented Oct 2, 2014

@Sophist-UK Thanks for the explanation, all clear to me now. Going to test ;)

@zero-24
Copy link
Contributor

zero-24 commented Oct 2, 2014

@roland-d As noted above i can't reproduce this #4369 (comment)

@roland-d
Copy link
Contributor

roland-d commented Oct 2, 2014

@test All good, tested it on Windows + FireFox and Chrome + IE and Mac + FF and Safari

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4369.

@infograf768
Copy link
Member

@test
No change here for display in Protostar

@Sophist-UK
Copy link
Contributor Author

The good news about all the tests on templates where the buttons already align ok is that changing the code doesn't misalign it.

They key point is that if we use consistent tags across all the buttons (which this PR does), then the buttons will definitely be aligned in all templates.

@roland-d
Copy link
Contributor

roland-d commented Oct 3, 2014

Indeed, there is no change in Protostar and that is the test that it is correct. Having consistent tags is in my opinion the real value in this PR.

@phproberto phproberto added the RTC This Pull Request is Ready To Commit label Oct 8, 2014
@phproberto phproberto closed this in 733641f Oct 8, 2014
phproberto pushed a commit that referenced this pull request Oct 8, 2014
@mbabker mbabker added this to the Joomla! 3.3.7 milestone Oct 10, 2014
@dgrammatiko
Copy link
Contributor

@Sophist-UK please check if #4561 works for you

@Sophist-UK
Copy link
Contributor Author

@dgt41 Sorry - but I am tied up with other stuff at present and won't be able to do this for you.

rdeutz pushed a commit to rdeutz/joomla-cms that referenced this pull request Oct 24, 2014
@mbabker mbabker modified the milestones: Joomla! 3.3.7, Joomla! 3.4.0 Nov 22, 2014
@zero-24 zero-24 removed the RTC This Pull Request is Ready To Commit label Oct 14, 2015
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

8 participants