Skip to content

Add a PGML tag block. #1042

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

Merged
merged 5 commits into from
Apr 10, 2024
Merged

Add a PGML tag block. #1042

merged 5 commits into from
Apr 10, 2024

Conversation

drgrice1
Copy link
Member

The syntax for a tag block is

[^ contents ^]{
    tag => 'tag name',
    attributes => { class => 'my-class', ... },
    tex_begin => 'tex start code',
    tex_end => 'tex end code'
}

or equivalently

[^ contents ^]{'tag name'}{{ class => 'my-class', ... }}{'tex start code'}{'tex end code'}

using the short form for passing options.

All of the options are optional (with the tag name defaulting to a 'div'). So you can do [^ hello ^], and that will render a div whose contents are "hello". The attibutes option should be a reference to a hash containing any HTML attributes you want the tag to have.

The contents of a tag are PGML. So you can do

[^
    [#
        [. [`x`] .] [. [`y`] .]*
        [. [^ [`1`] ^]{'span'}{{ style => 'color:blue' }} .] [. [`2`] .]*
    #]
^]

The above example also shows that a tag can be used in the cell of a niceTable.

Note that tag blocks can also contain other tag blocks.

The tex_begin and tex_end options are only used when the displayMode
is "TeX". The content will be wrapped in the values of those options
in that case. For example,

[^ My blue equation is [`x + y = 3`] ^]{'div'}{{ style => 'color:blue' }}{'\color{blue}'}

Note that when displayMode is "TeX", the contents (including the values of tex_begin and tex_end) are always wrapped in grouping braces. So you don't have to provide grouping (as would be needed for the color statement in the above example or any content after it would also be blue).

Note that the quotes on the tag name, tex_begin, and tex_end options are needed.

One of the main reasons for this new PGML block is to provide an easier way for a problem author to specify where the feedback for a MultiAnswer object with singleResult set should go. This is demonstrated in the following example:

DOCUMENT();
loadMacros('PGstandard.pl', 'PGML.pl', 'parserMultiAnswer.pl');

$multians = MultiAnswer(1, 4, 9)->with(
    singleResult => 1,
    checker      => sub {
        my ($correct, $student, $self, $ans) = @_;
        my $score = 0;
        for (0 .. $#$student) {
            $score += 1 if $correct->[$_] == $student->[$_];
        }
        return $score / @$correct;
    }
);

BEGIN_PGML
[^
    [#
        [. [`x`] .] [. [`x^2`] .]*{ headerrow => 1 }
        [. [`1`] .] [. [_]{$multians} .]*
        [. [`2`] .] [. [_]{$multians} .]*
        [. [`3`] .] [. [_]{$multians} .]
    #]{ valign => 'middle', padding => [ 0.5, 0.5 ] }
^]{'div'}{{ class => 'mx-auto ww-feedback-container ww-fb-align-middle' }}
END_PGML

ENDDOCUMENT();

Note that currently the contents of a tag block are rendered as if the tag block was not there for the "PTX" displayMode.

@drgrice1
Copy link
Member Author

I am sure there will still be issues to resolve with this, but I am submitting it to get feedback and see what is thought of this idea.

@drgrice1 drgrice1 requested review from dpvc and Alex-Jordan March 23, 2024 14:52
@Alex-Jordan
Copy link
Contributor

Can you think of a use for this beyond placing the feedback button? If it is only for placing the feedback button, it could be simpler and assume 'div' and maybe some other things. And it could be abstract (not be HTML-specific; not assume PG authors know about divs and classes).

What does this do for PTX output? (I think it should just do nothing there.)

@drgrice1
Copy link
Member Author

Yeah, I have lots of other uses for this. I would really like it to not assume a div.

As I stated, for "PTX" does nothing. It just renders the contents.

@drgrice1
Copy link
Member Author

Note that we have the openDiv and openSpan methods in PGbasicmacros.pl. This is better because it ensures tags are properly closed. Authors frequently as html to problems. This makes that convenient to do in the PGML setting.

@Alex-Jordan
Copy link
Contributor

I don't have objections to this. Although I am wary of encouraging authors to insert HTML tags unless they really know what they are doing. There will be a tendency to add some fancy feature to their PG problem that will work on screen, but fail for hardcopy.

These are random thoughts for consideration, not requests to change anything.

  • I know [< ... >] was reserved for links in the future, but maybe it's a better delimter than [^ ... ^] for a generic "tag".
  • Speaking of links, this could be used to make links, as long as the author can make the link in hardcopy and PTX at the same time.
  • What if the tag key were instead html, and then also have an (optional) ptx tag? Then that would create an issue with how to declare attributes for both html and ptx. But it could be like html => ['span', class => 'my-class'], ptx => ['alert']. In each case, a simple string instead of an array ref would also be understood.
  • Instead of tex_begin, tex_end, there could just be tex => [ , ] as a way to enforce authors thinking about what is needed at both ends.
  • What if tag (or html if it changes) only allowed a finite list of tags? As in, whitelist div, span, a, and other things. Don't allow script (unless that's one you wanted). Don't allow iframe. The thinking being don't allow things that you really have little-to-no hope will work out in hardcopy or PTX.

@drgrice1
Copy link
Member Author

* I know `[< ... >]` was reserved for links in the future, but maybe it's a better delimter than `[^ ... ^]` for a generic "tag".

I certainly wanted to use [< ... >] as that is certainly intuitive for this, but didn't for exactly this reason. I will switch it if that would be okay.

* Speaking of links, this could be used to make links, as long as the author can make the link in hardcopy and PTX at the same time.

* What if the `tag` key were instead `html`, and then also have an (optional) `ptx` tag? Then that would create an issue with how to declare attributes for both html and ptx. But it could be like `html => ['span', class => 'my-class'],  ptx => ['alert']`. In each case, a simple string instead of an array ref would also be understood.

That might work. So you are suggesting the syntax be [^ ... ^]{ html => ['span', class => 'my-class'], tex => ['tex begin', 'tex end'], ptx => ['alert'] } and the short form then would be [^ ... ^]{['span', class => 'my-class']}{['tex begin', 'tex_end']}{['alert']} (of course this takes into account the next comment). Correct?

* Instead of `tex_begin, tex_end`, there could just be `tex => [ , ]` as a way to enforce authors thinking about what is needed at both ends.

Excellent. I should have thought of that. I will definitely switch to that.

* What if `tag` (or `html` if it changes) only allowed a finite list of tags? As in, whitelist `div`, `span`, `a`, and other things. Don't allow `script` (unless that's one you wanted). Don't allow `iframe`. The thinking being don't allow things that you really have little-to-no hope will work out in hardcopy or PTX.

I would be fine with limitations. We just need to settle on a list of allowed tags. I certainly want div and span, and if this is switched to [< ... >] then a should be good. Maybe p should be in the list also, although maybe not since that is not so generic and should be handled in a way that is consistent for all display modes. I agree that script and iframe are not needed.

@Alex-Jordan
Copy link
Contributor

So you are suggesting the syntax be [^ ... ^]{ html => ['span', class => 'my-class'], tex => ['tex begin', 'tex end'], ptx => ['alert'] } and the short form then would be [^ ... ^]{['span', class => 'my-class']}{['tex begin', 'tex_end']}{['alert']}

Yes, but also if there are no attributes you could have a shortcut, dispensing with the brackets:

[^ ... ^]{ html => 'span', tex => ['tex begin', 'tex end'], ptx => 'alert' }
[^ ... ^]{'span'}{['tex begin', 'tex_end']}{'alert'}

@drgrice1
Copy link
Member Author

Yeah, I got that. Just wanted to check that I understood your format correctly.

@Alex-Jordan
Copy link
Contributor

I suppose you could allow a shortcut string argument for tex as well, if you assume it is an environment name. Like

tex => 'quote'

is equivalent to

tex => ['\begin{quote}', '\end{quote}'].

@drgrice1
Copy link
Member Author

This is now switched from [^ ... ^] to [< ... >] for tags.

The general syntax is now

[< content >]{
    html => [ 'tag_name', class => 'my-class', ... ],
    tex  => [ 'tex begin', 'tex end' ],
    ptx  => [ 'tag name', { attribute => 1, ... }, 'separator' ]
}

or

[< content >]{['tag_name', class => 'my-class', ...]}{['tex begin', 'tex end']}{['tag name', { attribute => 1, ... }, 'separator']}

The html, tex, and ptx options can also just be strings. For html and ptx that string will be the tag name. If the tex argument is a string then the contents will be wrapped in a TeX environment by that name.

Note that for html when the argument is given as an array, the tag name can also be omitted. In that case a div tag will be used. So you can do [< content >]{[ class => 'my-class' ]}.

A link can be created with this as well. For example, [<Google>]{[ 'a', href => 'https://www.google.com' ]}.

I would be willing to switch the default to being an a tag. So then to get a div you would need [< content >]{[ 'div', class => 'my-class' ]}, but to obtain a link you could do [<Google>]{[ href => 'https://www.google.com' ]}. That would make the [< ... >] block more like the originaly intended link. I left the default as a div because that usage is probably nicer for what I would use this for (I never put links in problems).

For now the only allowed HTML tags are div, span, and a. What other tags should be allowed?

Note that for PTX display mode this uses the NiceTables::tag method. I am not sure if that was a good idea, but it was an idea. @Alex-Jordan: Please advise.

@drgrice1 drgrice1 force-pushed the pgml-tag branch 4 times, most recently from 0fcd097 to ce307ef Compare March 27, 2024 22:02
@Alex-Jordan
Copy link
Contributor

Tested and it all seems good.

I am not able to think of a reason why an author would want to specify a separator, but there may be some PTX tags where it really matters if there is a newline, space, empty string, etc. So it could be helpful to have that, for a small price of having to use braces and not having symmetric syntax with html. In other words, I think it is good that you used NiceTables::tag.

The unfortunate thing about using this for links is that you have to repeat yourself three times for html, tex, and ptx if you consistently want a link in each output format. A different idea from changing the default from div to a, would be that if an href attribute is specified, that overrules anything the other three might be asking for. And you get the appropriate think for a link in all three places. So like:

[< content >]{
    href => 'https://openwebwork.org/',
    html => irrelevant,
    tex  => irrelevant,
    ptx  => irrelevant
}

I'm not sure what to do with the short form then. Can you get away with like:

[< content >]{}{}{}{'https://openwebwork.org/'}

?

This next idea doesn't feel quite right, but if we are already limiting the value for html => to just be 'div' or 'span', then anything that is not one of those strings could be interpreted as an href. So then you could have like:

[< content >]{
    html => 'https://openwebwork.org/',
    tex  => irrelevant,
    ptx  => irrelevant
}

with short form:

[< content >]{'https://openwebwork.org/'}

Or perhaps just leave it the way it is now, but if the html value is an a with an href, then overrule whatever is passed for the other two arguments and make them links as well.

[< content >]{
    html => ['a', href => 'https://openwebwork.org/'],
    tex  => irrelevant,
    ptx  => irrelevant
}

with short form:

[< content >]{['a', href => 'https://openwebwork.org/']}

I'm not sure if any of these ideas are good. Maybe we should just do links separately. Somewhat inspired by markdown, like [(link text)]{href here}

@drgrice1
Copy link
Member Author

I also don't like the need to specify all three cases for links, and I noticed that as well.

I think the nicest choice is to use separate markdown for tags and links. So for tags the markdown would stay as it is in this pull request except not allowing the a tag. Then [(link text)]{href} would be used for links.

Although, I would be okay with keeping the current structure, and handling link tags specially as in your last example.

I think that the first two examples seem rather tedious for usage and probably for implementation as well.

@dpvc
Copy link
Member

dpvc commented Mar 28, 2024 via email

@drgrice1
Copy link
Member Author

[< content >]{ href => 'https://openwebwork.org/', html => irrelevant, tex => irrelevant, ptx => irrelevant }
Of course, you don't need to provide the irrelevant values, so [< content >]{href => 'https://openwebwork.org'} would be sufficient.

I think @Alex-Jordan knows that the later options are not needed in that case, but he was providing them to emphasize what the options would be.

Then [(link text)]{href} would be used for links.
I'm wondering if [[link text]]{href} might not be better, as Markdown uses [link text](url) for links. Davide

Sounds natural.

@Alex-Jordan
Copy link
Contributor

That all sounds good, and it feels right to just someday have separate link markdown.

And yes, I just wanted to stress in those examples that the presence of an href would render any other options irrelevant, if they happen to be there. But an author would not normally have them there.

I have no preferences about delimiters for a future link markdown (which would be in a separate PR). I was thinking "Markdown uses brackets and parentheses" when I suggested [( ... )]. But also Markdown puts the link text in the brackets, so [[ ... ]] makes sense.

@drgrice1
Copy link
Member Author

I removed the a tag from the whitelist for the new tag block. So now only divs and spans are allowed. The link block can be added in another pull request (per @Alex-Jordan's request).

@pstaabp
Copy link
Member

pstaabp commented Mar 30, 2024

Just doing some testing here. Returning to span example, it appears this is the notation

[< This is some math [`x+y=9`]>]{
html => ['span', style => 'color:blue'],
tex => ['{\color{blue}', '}']
}

to ensure hardcopy is working.

I do agree that making sure that authors are writing proper hardcopy will be difficult, but leaving off the tex option, will still compile for tex, just won't create a blue span.

@Alex-Jordan
Copy link
Contributor

Alex-Jordan commented Mar 30, 2024 via email

Copy link
Contributor

@Alex-Jordan Alex-Jordan left a comment

Choose a reason for hiding this comment

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

My last comment is just an observation. Authors who are declaring div or span ought to know what they are doing.

@somiaj
Copy link
Contributor

somiaj commented Mar 30, 2024

Would it be worth adding aliases for common tags that might get used? Though this may just end up creating a mess like the old pg tagging variables, but thinking something like @pstaabp example, so instead of having to write:

[< This is some math [`x+y=9`]>]{
html => ['span', style => 'color:blue'],
tex => ['{\color{blue}', '}']
}

one could instead just write [< This is some math ['x+y=9']>]{blue}, and the html, tex, and ptx for this could already be known.

@Alex-Jordan
Copy link
Contributor

Alex-Jordan commented Mar 30, 2024 via email

@drgrice1
Copy link
Member Author

The example with a span with color was only meant to demonstrate the usage, and was used because the style => 'color:blue' was the first style attribute that popped into my head that gave something visible to show that it worked. It was not meant to be something that demonstrates good problem authoring practice.

@Alex-Jordan
Copy link
Contributor

Alex-Jordan commented Mar 30, 2024 via email

@drgrice1
Copy link
Member Author

I haven't tested the following, but I guess this lets authors make invalid html. Like putting a span around a table. Do we care?

I added a check to prevent this sort of thing. It is still possible for a span tag to be created with invalid contents and it is not possible to prevent that entirely, but the last change prevents that from being done by adding a PGML block in a span tag block.

@drgrice1
Copy link
Member Author

So in particular, a span tag block can not contain a table tag block.

@somiaj
Copy link
Contributor

somiaj commented Mar 30, 2024

@Alex-Jordan I guess most things that would produce good output are already included in PGML. I can't think of anything right now that would fit that list.

Another thought would be an alias system that users could set up aliases for common tags they use, then just reference that tag later using the alias. This way they don't have to include all options each time the tag is used. Also, if there were some default alias we decided would be useful, they could be in there at a start.

Just some thoughts I had to avoid having to write out all three modes of output in each tag (though this is assuming this would be used a lot).

drgrice1 added 5 commits April 7, 2024 17:25
The syntax for a tag block is

```
[^ contents ^]{
    tag => 'tag name',
    attributes => { class => 'my-class', ... },
    tex_begin => 'tex start code',
    tex_end => 'tex end code'
}
```

or equivalently

`[^ contents ^]{'tag name'}{{ class => 'my-class', ... }}{'tex start code'}{'tex end code'}`

using the short form for passing options.

All of the options are optional (with the tag name defaulting to a
'div').  So you can do `[^ hello ^]`, and that will render a `div` whose
contents are "hello".  The `attibutes` option should be a reference to a
hash containing any HTML attributes you want the tag to have.

The contents of a tag are PGML.  So you can do

```
[^
    [#
        [. [`x`] .] [. [`y`] .]*
        [. [^ [`1`] ^]{'span'}{{ style => 'color:blue' }} .] [. [`2`] .]*
    #]
^]
```

The above example also shows that a tag can be used in the cell of a
niceTable.

Note that tag blocks can also contain other tag blocks.

The `tex_begin` and `tex_end` options are only used when the displayMode
is "TeX".   The content will be wrapped in the values of those options
in that case.  For example,

```
[^ My blue equation is [`x + y = 3`] ^]{'div'}{{ style => 'color:blue' }}{'\color{blue}'}
```

Note that when displayMode is "TeX", the contents (including the values
of `tex_begin` and `tex_end`) are always wrapped in grouping braces.  So
you don't have to provide grouping (as would be needed for the color
statement in the above example or any content after it would also be
blue).

Note that the quotes on the tag name, tex_begin, and tex_end options are
needed.

One of the main reasons for this new PGML block is to provide an easier
way for a problem author to specify where the feedback for a MultiAnswer
object with singleResult set should go.  This is demonstrated in the
following example:

```
DOCUMENT();
loadMacros('PGstandard.pl', 'PGML.pl', 'parserMultiAnswer.pl');

$multians = MultiAnswer(1, 4, 9)->with(
    singleResult => 1,
    checker      => sub {
        my ($correct, $student, $self, $ans) = @_;
        my $score = 0;
        for (0 .. $#$student) {
            $score += 1 if $correct->[$_] == $student->[$_];
        }
        return $score / @$correct;
    }
);

BEGIN_PGML
[^
    [#
        [. [`x`] .] [. [`x^2`] .]*{ headerrow => 1 }
        [. [`1`] .] [. [_]{$multians} .]*
        [. [`2`] .] [. [_]{$multians} .]*
        [. [`3`] .] [. [_]{$multians} .]
    #]{ valign => 'middle', padding => [ 0.5, 0.5 ] }
^]{'div'}{{ class => 'mx-auto ww-feedback-container ww-fb-align-middle' }}
END_PGML

ENDDOCUMENT();
```

Note that currently the contents of a tag block are rendered as if the
tag block was not there for the "PTX" displayMode.
Change the general syntax to

```
[< content >]{
    html => [ 'tag_name', class => 'my-class', ... ],
    tex  => [ 'tex begin', 'tex end' ],
    ptx  => [ 'tag name', { attribute => 1, ... }, 'separator' ]
}
```

or

`[< content >]{['tag_name', class => 'my-class', ...]}{['tex begin', 'tex end']}{['tag name', { attribute => 1, ... }, 'separator']}`

The `html`, `tex`, and `ptx` options can also just be strings.  For
`html` and `ptx` that string will be the tag name.  If the `tex`
argument is a string then the contents will be wrapped in a TeX
environment by that name.

Note that for `html` when the argument is given as an array, the tag
name can also be omitted.  In that case a `div` tag will be used.  So
you can do `[< content >]{[ class => 'my-class' ]}`.

A link can be created with this as well.  For example,
`[<Google>]{[ 'a', href => 'https://www.google.com' ]}`.

I would be willing to switch the default to being an `a` tag.  So then
to get a `div` you would need `[< content >]{[ 'div', class => 'my-class' ]}`,
but to obtain a link you could do
`[<Google>]{[ href => 'https://www.google.com' ]}`.  That would make
the `[< ... >]` block more like the originaly intended link. I left the
default as a `div` because that usage is probably nicer for what I would
use this for (I never put links in problems).

For now the only allowed HTML tags are `div`, `span`, and `a`.  What
other tags should be allowed?

Note that for `PTX` display mode this uses the `NiceTables::tag` method.
I am not sure if that was a good idea, but it was an idea.
@Alex-Jordan: Please advise.
This prevents new lines from working correctly inside a div tag.
A span tag block is not allowed to contain any PGML blocks that have the
type indent, align, par, list, bullet, answer, heading, rule, code, pre,
verbatim, table, or tag.  If a span tag block is detected with any of
those things in it, a warning is issued and the contents rendered
directly without the span tag.  This helps to prevent problem authors
from doing the wrong thing.

It is valid for a span to contain a span, but if a span tag block
contains another tag bloc, the outer span tag block does not have the
information to determine what the inner tag block is (span or div).
Since I don't think it is particularly useful to have a span within a
span, I just blocked any tag block in a span.
@pstaabp pstaabp merged commit 97fa82a into openwebwork:develop Apr 10, 2024
@drgrice1 drgrice1 deleted the pgml-tag branch April 10, 2024 20:29
@Alex-Jordan
Copy link
Contributor

As I reviewed openwebwork/webwork2#2397, I wondered if there should be an addition to PGML Lab about the tag block. On the one hand, it is a bit abstract. But under the "Answers" category, the technique of positioning the feedback button could be demonstrated in an example. I can do this if you agree. I wanted to check if you'd rather describe it in a more general way.

@drgrice1
Copy link
Member Author

@Alex-Jordan: That is done in openwebwork/webwork2#2397 (which has now been merged).

@Alex-Jordan
Copy link
Contributor

Oh, sorry I missed that! You described it in the PR, and I didn't think to look in the Substitutions category.

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

Successfully merging this pull request may close these issues.

5 participants