Changed default tags to <mark> (from <strong>) in the highlight_phrase() method in text helper #1497

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
@alexbilbie
Contributor

alexbilbie commented Jun 17, 2012

I've changed the default tag for the highlight_phrase() method in the text helper from <strong> to <mark>.

The <mark> tag semantically represents highlighted text in HTML5. The spec details can be found here http://www.w3.org/TR/html5/the-mark-element.html#the-mark-element

@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Jun 17, 2012

Contributor

Not sure it's appropriate to set it as default at this point, considering it's HTML 5.

Contributor

narfbg commented Jun 17, 2012

Not sure it's appropriate to set it as default at this point, considering it's HTML 5.

@alexbilbie

This comment has been minimized.

Show comment Hide comment
@alexbilbie

alexbilbie Jun 18, 2012

Contributor

I don't think because it is HTML5 it should stop it being used. It's semantically valid and the appropriate tag for this use case. Also browsers that don't understand the tag natively will still render the text inside it.

What do you think @philsturgeon @derekjones @pkriete @ericbarnes ?

Contributor

alexbilbie commented Jun 18, 2012

I don't think because it is HTML5 it should stop it being used. It's semantically valid and the appropriate tag for this use case. Also browsers that don't understand the tag natively will still render the text inside it.

What do you think @philsturgeon @derekjones @pkriete @ericbarnes ?

@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Jun 18, 2012

Contributor

If it doesn't exist in the HTML specifications prior to HTML 5, then it wouldn't be semantically valid for them. And while the text will be rendered - it won't be highlighted.

Contributor

narfbg commented Jun 18, 2012

If it doesn't exist in the HTML specifications prior to HTML 5, then it wouldn't be semantically valid for them. And while the text will be rendered - it won't be highlighted.

@alexbilbie

This comment has been minimized.

Show comment Hide comment
@alexbilbie

alexbilbie Jun 18, 2012

Contributor

But then if we're going to wait for the HTML5 specification to be complete (and the section with the <mark> tag has been finalised) this pull request is going to be open until 2020.

Contributor

alexbilbie commented Jun 18, 2012

But then if we're going to wait for the HTML5 specification to be complete (and the section with the <mark> tag has been finalised) this pull request is going to be open until 2020.

@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Jun 18, 2012

Contributor

It is an optional parameter, so everybody can change it to use the <mark> tag if they're developing an HTML5 application. It's just that changing the default might break it for existing apps that are not HTML5-ready.

Another thought however - I'm completely out of touch with the feature/html5 branch, but if there's some configuration setting or auto-detection on wether we use HTML 5 - it could be changed to depend on that in there.

Contributor

narfbg commented Jun 18, 2012

It is an optional parameter, so everybody can change it to use the <mark> tag if they're developing an HTML5 application. It's just that changing the default might break it for existing apps that are not HTML5-ready.

Another thought however - I'm completely out of touch with the feature/html5 branch, but if there's some configuration setting or auto-detection on wether we use HTML 5 - it could be changed to depend on that in there.

@alexbilbie

This comment has been minimized.

Show comment Hide comment
@alexbilbie

alexbilbie Jun 18, 2012

Contributor

True that users can currently set whichever tag they want in the function - but it's currently a <strong> which is now, with the advent of the <mark> tag, is the incorrect tag to use.

As for the auto-detection I strongly believe that is the wrong approach to go down

Contributor

alexbilbie commented Jun 18, 2012

True that users can currently set whichever tag they want in the function - but it's currently a <strong> which is now, with the advent of the <mark> tag, is the incorrect tag to use.

As for the auto-detection I strongly believe that is the wrong approach to go down

@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Jun 18, 2012

Contributor

I'm not arguing on what is right or wrong, I'm just saying we have to maintain backwards-compatibility. :)

Contributor

narfbg commented Jun 18, 2012

I'm not arguing on what is right or wrong, I'm just saying we have to maintain backwards-compatibility. :)

@vkeranov

This comment has been minimized.

Show comment Hide comment
@vkeranov

vkeranov Jun 18, 2012

Contributor

make it HTML5 :)
Most of the browsers already support it. We don't need to think about IE6 still, don't we? Besides, everybody can do:

mark { font-weight: bolder; }
Contributor

vkeranov commented Jun 18, 2012

make it HTML5 :)
Most of the browsers already support it. We don't need to think about IE6 still, don't we? Besides, everybody can do:

mark { font-weight: bolder; }
@ericbarnes

This comment has been minimized.

Show comment Hide comment
@ericbarnes

ericbarnes Jun 18, 2012

Contributor

My opinion is we should start converting all helpers that generate html to html5.

Contributor

ericbarnes commented Jun 18, 2012

My opinion is we should start converting all helpers that generate html to html5.

@tkaw220

This comment has been minimized.

Show comment Hide comment
@tkaw220

tkaw220 Jun 18, 2012

Contributor

There're still users who simply can't or don't know how to move away from IE6. Backwards-compatibility won't hurt in this case IMHO.

Contributor

tkaw220 commented Jun 18, 2012

There're still users who simply can't or don't know how to move away from IE6. Backwards-compatibility won't hurt in this case IMHO.

@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Jun 18, 2012

Contributor

I'm not talking about IE6 users, even Microsoft badly wants the IE6 problem to go away. This one isn't (directly) related to browsers' market share - it's about the already existing CodeIgniter applications. Correct me if I'm wrong, but to my knowledge no standard prior to HTML 5 has the <mark> tag, which means that it would break validation for those standards, while <strong> (I assume) is valid for HTML 5 as well.

I'm not against pushing for a wider HTML 5 adoption - I'm all for it, but it should happen in a less painful way.

Contributor

narfbg commented Jun 18, 2012

I'm not talking about IE6 users, even Microsoft badly wants the IE6 problem to go away. This one isn't (directly) related to browsers' market share - it's about the already existing CodeIgniter applications. Correct me if I'm wrong, but to my knowledge no standard prior to HTML 5 has the <mark> tag, which means that it would break validation for those standards, while <strong> (I assume) is valid for HTML 5 as well.

I'm not against pushing for a wider HTML 5 adoption - I'm all for it, but it should happen in a less painful way.

@derekjones

This comment has been minimized.

Show comment Hide comment
@derekjones

derekjones Jun 18, 2012

Contributor

EllisLab no longer supports IE6 at all, FYI. That's not a determining factor here, but I thought I'd mention it in case there was worry that we might demand that it work with dead browsers.

One solution would be to move CI's helpers to HTML 5, and maintain a separate repo of helpers for HTML 4 versions for those that need them.

Contributor

derekjones commented Jun 18, 2012

EllisLab no longer supports IE6 at all, FYI. That's not a determining factor here, but I thought I'd mention it in case there was worry that we might demand that it work with dead browsers.

One solution would be to move CI's helpers to HTML 5, and maintain a separate repo of helpers for HTML 4 versions for those that need them.

@alexbilbie

This comment has been minimized.

Show comment Hide comment
@alexbilbie

alexbilbie Jun 18, 2012

Contributor

In 3d8d4fb I've added a note on how to add styling for browsers which may not have default styling for <mark>

Contributor

alexbilbie commented Jun 18, 2012

In 3d8d4fb I've added a note on how to add styling for browsers which may not have default styling for <mark>

@hArpanet

This comment has been minimized.

Show comment Hide comment
@hArpanet

hArpanet Jul 1, 2012

Contributor

I'm apathetic to this pull request. The point of a 'default' value is that it is the most commonly used value. With the relative newness of HTML 5 I can't see that moving from < strong > to < mark > can be the most commonly used value.

I'm not against this request either as I can see it makes sense, however, this pull request has stimulated a discussion about HTML 5 not about whether the default needs changing, and I believe it would be better to have a proper discussion elsewhere and an agreed date as to when CI implements all 'common' elements of HTML 5, rather than it being a debate on each pull request of this type. Decide when HTML 5 becomes the CI standard and then implement all these pulls.

It wouldn't hurt either if the docs specifically highlighted when and where HTML 5 features had been used. (Note: I had to use < strong > tags here as < mark > tags are not implemented!)

Contributor

hArpanet commented Jul 1, 2012

I'm apathetic to this pull request. The point of a 'default' value is that it is the most commonly used value. With the relative newness of HTML 5 I can't see that moving from < strong > to < mark > can be the most commonly used value.

I'm not against this request either as I can see it makes sense, however, this pull request has stimulated a discussion about HTML 5 not about whether the default needs changing, and I believe it would be better to have a proper discussion elsewhere and an agreed date as to when CI implements all 'common' elements of HTML 5, rather than it being a debate on each pull request of this type. Decide when HTML 5 becomes the CI standard and then implement all these pulls.

It wouldn't hurt either if the docs specifically highlighted when and where HTML 5 features had been used. (Note: I had to use < strong > tags here as < mark > tags are not implemented!)

@alexbilbie

This comment has been minimized.

Show comment Hide comment
@alexbilbie

alexbilbie Jul 31, 2012

Contributor

I've updated this pull so it will auto merge and it now includes an upgrade note.

So what's the decision on introducing HTML5 into CI?

Contributor

alexbilbie commented Jul 31, 2012

I've updated this pull so it will auto merge and it now includes an upgrade note.

So what's the decision on introducing HTML5 into CI?

@philsturgeon

This comment has been minimized.

Show comment Hide comment
@philsturgeon

philsturgeon Jul 31, 2012

Contributor

HTML5 is king?

Phil Sturgeon

On Tuesday, 31 July 2012 at 13:09, Alex Bilbie wrote:

I've updated this pull so it will auto merge and it now includes an upgrade note.

So what's the decision on introducing HTML5 into CI?


Reply to this email directly or view it on GitHub:
EllisLab#1497 (comment)

Contributor

philsturgeon commented Jul 31, 2012

HTML5 is king?

Phil Sturgeon

On Tuesday, 31 July 2012 at 13:09, Alex Bilbie wrote:

I've updated this pull so it will auto merge and it now includes an upgrade note.

So what's the decision on introducing HTML5 into CI?


Reply to this email directly or view it on GitHub:
EllisLab#1497 (comment)

@alexbilbie

This comment has been minimized.

Show comment Hide comment
@alexbilbie

alexbilbie Jul 31, 2012

Contributor

In the IRC people have suggested we have a $config['html5'] flag.

Personally I'm against this however if others think this is a better option then can I suggest we have it set to TRUE by default

Contributor

alexbilbie commented Jul 31, 2012

In the IRC people have suggested we have a $config['html5'] flag.

Personally I'm against this however if others think this is a better option then can I suggest we have it set to TRUE by default

@philsturgeon

This comment has been minimized.

Show comment Hide comment
@philsturgeon

philsturgeon Jul 31, 2012

Contributor

I believe we talked before about having "doctype" or some other co fig switch that could be set to "xhtml" or "html5" or whatever, but as long as its optional I'm not too fussed.

Emailing from my iPhone 4S like a BOSS!

On 31 Jul 2012, at 13:52, Alex Bilbiereply@reply.github.com wrote:

In the IRC people have suggested we have a $config['html5'] flag.

Personally I'm against this however if others think this is a better option then can I suggest we have it set to TRUE by default


Reply to this email directly or view it on GitHub:
EllisLab#1497 (comment)

Contributor

philsturgeon commented Jul 31, 2012

I believe we talked before about having "doctype" or some other co fig switch that could be set to "xhtml" or "html5" or whatever, but as long as its optional I'm not too fussed.

Emailing from my iPhone 4S like a BOSS!

On 31 Jul 2012, at 13:52, Alex Bilbiereply@reply.github.com wrote:

In the IRC people have suggested we have a $config['html5'] flag.

Personally I'm against this however if others think this is a better option then can I suggest we have it set to TRUE by default


Reply to this email directly or view it on GitHub:
EllisLab#1497 (comment)

@alexbilbie

This comment has been minimized.

Show comment Hide comment
@alexbilbie

alexbilbie Jul 31, 2012

Contributor
<?php
function highlight_phrase($str, $phrase, $tag_open = '', $tag_close = '')
{
    if ($tag_open === '' && $tag_close === '')
    {
        $tag_open = (config_item('HTML5')) ? '<mark>' : '<strong>';
        $tag_close = (config_item('HTML5')) ? '</mark>' : '</strong>';
    }

    if ($str === '')
    {
        return '';
    }

    if ($phrase !== '')
    {
        return preg_replace('/('.preg_quote($phrase, '/').')/i', $tag_open.'\\1'.$tag_close, $str);
    }

    return $str;
}
Contributor

alexbilbie commented Jul 31, 2012

<?php
function highlight_phrase($str, $phrase, $tag_open = '', $tag_close = '')
{
    if ($tag_open === '' && $tag_close === '')
    {
        $tag_open = (config_item('HTML5')) ? '<mark>' : '<strong>';
        $tag_close = (config_item('HTML5')) ? '</mark>' : '</strong>';
    }

    if ($str === '')
    {
        return '';
    }

    if ($phrase !== '')
    {
        return preg_replace('/('.preg_quote($phrase, '/').')/i', $tag_open.'\\1'.$tag_close, $str);
    }

    return $str;
}
@ckdarby

This comment has been minimized.

Show comment Hide comment
@ckdarby

ckdarby Jan 3, 2013

Contributor

@alexbilbie Where does this stand?

Contributor

ckdarby commented Jan 3, 2013

@alexbilbie Where does this stand?

@marcvdm

This comment has been minimized.

Show comment Hide comment
@marcvdm

marcvdm Jun 1, 2013

I think that the config option is great way to start introducing html5. Just make an option where you can choose between html5 and normal and make it default to normal.

That way future html5 change requests can make use of this config.

You will still be backwards compatible and yet give people the choice for html5

marcvdm commented Jun 1, 2013

I think that the config option is great way to start introducing html5. Just make an option where you can choose between html5 and normal and make it default to normal.

That way future html5 change requests can make use of this config.

You will still be backwards compatible and yet give people the choice for html5

@ivantcholakov

This comment has been minimized.

Show comment Hide comment
@ivantcholakov

ivantcholakov Jul 30, 2013

Contributor

The current default values '<strong>' and '</strong>' do not have much sense. The author of the original text most probably would "occupy" these tags and when the system highlights a phrase surrounded with "strong" tag, it would not be visible. A wise developer would already need to add a specific class, for example '<strong class="highlight">'.

This is why a change of these default values would not be such a pain. Anyway, within the instruction for upgrade, those who want to use the "strong" tag might be advised to declare highlight_phrase() with the old defaults within MY_text_helper.php

Autodetection for HTML5 is a messy approach IMO.

Anyway, we are going to swallow the new class and file naming convention, so this change is not so big concern about backward compatibility. The <mark> tag really makes sense.

Contributor

ivantcholakov commented Jul 30, 2013

The current default values '<strong>' and '</strong>' do not have much sense. The author of the original text most probably would "occupy" these tags and when the system highlights a phrase surrounded with "strong" tag, it would not be visible. A wise developer would already need to add a specific class, for example '<strong class="highlight">'.

This is why a change of these default values would not be such a pain. Anyway, within the instruction for upgrade, those who want to use the "strong" tag might be advised to declare highlight_phrase() with the old defaults within MY_text_helper.php

Autodetection for HTML5 is a messy approach IMO.

Anyway, we are going to swallow the new class and file naming convention, so this change is not so big concern about backward compatibility. The <mark> tag really makes sense.

@ivantcholakov

This comment has been minimized.

Show comment Hide comment
@ivantcholakov

ivantcholakov Jul 30, 2013

Contributor

+1 for the initially proposed change.

<mark>This <strong>example</mark> works</strong> for me (Opera 12.16, IE 10, Firefox 22.0, Crome 28, Safari 5.1.7).

Contributor

ivantcholakov commented Jul 30, 2013

+1 for the initially proposed change.

<mark>This <strong>example</mark> works</strong> for me (Opera 12.16, IE 10, Firefox 22.0, Crome 28, Safari 5.1.7).

narfbg added a commit that referenced this pull request Jan 7, 2014

@narfbg narfbg closed this Jan 7, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment