Problem/Motivation

I'm working on my GSoC project for CKEditor plugins - displaying segments of texts and masking the HTML tags inside the segments.
We want to extend TMGMT by adding new functionalities - segments as pieces of text, which will be integrated with tmgmt memory to provide translation suggestions, usages, etc. One of the blockers we found now is that we don't support the HTML tags inside the segments.

More info in my blog posts can be found here.

Proposed resolution

Discuss if and how should TMGMT support the masking of HTML tags inside the editor and implement it.

Remaining tasks

User interface changes

API changes

Data model changes

Comments

sasanikolic created an issue. See original summary.

miro_dietiker’s picture

We discussed this requirement and it seems clear that TMGMT need to provide support for masking while editing.

So if we have an Editor that need tag masking, it means
- The text area will not show the original values. Instead it will show the masked values.
- On submission, the masking is reverted.

For this, two hooks should be provided:
- One that is triggered before we output the values in the form.
- One that is triggered when the values are taken from form state and applied to the job/data item.
This allows easy implementation of the ckeditor integration.

Mind that both hooks will receive the alterable variable with the single data text. And additionally, they will get job item / job context so they know what they have to do.

Berdir’s picture

They will get *two* contexts. $data_item and $job_item.

sasanikolic’s picture

This issue is about mapping the data from source to the translation editor. I think we should open another one for tag masking.

miro_dietiker’s picture

Yeah sorry i thought after our discussion, the description was incomplete.

Now that most of the discussion about the data mangling hooks is here, why not create a separate issue to discuss the mapping?

sasanikolic’s picture

Title: Provide an extension to map source data into translation editor » Provide support for masking HTML tags while editing
Issue summary: View changes
sasanikolic’s picture

As per our discussion, we redefined the tags structure.

If the the tag starts with <b>, without any attributes, the masked tag should look like this:
<tmgmt-tag element=”b” raw=”&lt;b&gt;”>

Or, if the html element, for example starts with <img alt=... title=...> - has attributes, the tag would like this:
<tmgmt-tag element=”img” raw=”&lt;img src=... alt=... title=...&gt;”>

In the examples above, element represents the name of the html tag, while the raw is the encoded tag. Don't forget about the closing tags. When unmasking, the process will simply replace tmgmt-tag with its raw property.

With this implementation though, the alt and title attributes could not be translated and be placed back/saved untranslated, even though they could be translatable. We'd need to find a proper solution for this.

We'd also need tests for this two possibilities.

tduong’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
4.85 KB

Here is a starting patch, to see if this approach works good. Still need to discuss about considering to add a context to distinguish source from translation and if the unmask hook is placed in the right place.
And of course still need test coverage.

miro_dietiker’s picture

Status: Needs review » Needs work
  1. +++ b/src/Form/JobItemForm.php
    @@ -346,14 +347,19 @@ class JobItemForm extends TmgmtFormBase {
    +          $text = $data_item['#translation']['#text'];
    

    Hmm, we extract $text and allow altering it, but it is later not used?! It should be attached to $data IMHO otherwise it won't have any effect?

  2. +++ b/src/Form/JobItemForm.php
    @@ -346,14 +347,19 @@ class JobItemForm extends TmgmtFormBase {
    +          \Drupal::moduleHandler()->alter('tmgmt_data_item_text_input', $text, $data_item, $item);
    
    @@ -962,10 +968,16 @@ class JobItemForm extends TmgmtFormBase {
    +    $text = NULL;
    ...
    +      \Drupal::moduleHandler()->alter('tmgmt_data_item_text_output', $text, $data_item, $this->entity);
    

    I think the two hooks need documentation in .api file.

tduong’s picture

Status: Needs work » Needs review
FileSize
5.31 KB
8.7 KB

Done + starting test coverage.

Next step will be to implement some real masking as part of @sasanikolic's project as a pull request on github against his project, so we have a real case before committing the patch. Will do this on monday.

miro_dietiker’s picture

Status: Needs review » Needs work

Looks pretty nice.

+++ b/src/Form/JobItemForm.php
@@ -962,10 +958,16 @@ class JobItemForm extends TmgmtFormBase {
+      \Drupal::moduleHandler()->alter('tmgmt_data_item_text_output', $text, $data_item, $this->entity);

I think we should always trigger this alter...
The case where translation is empty could be important to initialise it with empty segment-only tags or filled with source text and special indication "needs translation".

tduong’s picture

Assigned: Unassigned » tduong
Status: Needs work » Needs review
FileSize
2.08 KB
9.77 KB

I've just added a check where it could be possible to handle the tags thing for this empty case. Now I'll create a PR in the github project to provide a real example.

tduong’s picture

Started with some regex to mask/unmask the hardcoded b- and img-tags. Created PR here. Still work in progress.

miro_dietiker’s picture

Status: Needs review » Needs work
+++ b/src/Entity/Job.php
@@ -708,12 +708,7 @@ class Job extends ContentEntityBase implements EntityOwnerInterface, JobInterfac
-      $this->submitted();
...
+    $this->getTranslatorPlugin()->requestTranslation($this);

Accidental revert of 9d74ad9

tduong’s picture

Status: Needs work » Needs review
FileSize
625 bytes
9.01 KB

Sorry, forgot to update my feature branch :P

miro_dietiker’s picture

Status: Needs review » Needs work

OK, discussed and decided...

We learned with the segmenter and the editors that we should think in source + translation pairs.
Thus the hook should also operate on the pair level.

See JobItemForm::reviewFormElement()

        // Build source and translation areas.
        $item_element = $this->buildSource($item_element, $data_item, $rows, $form_state);
        $item_element = $this->buildTranslation($item_element, $data_item, $rows, $form_state, $is_preliminary);

The hook should fire before buildSource and buildTranslation, and the modified masked text should be passed into these methods as a new argument.

The submission only needs to operate on the translation. Unmasking source is not needed.
And then, we can finally commit this. :-)

tduong’s picture

Status: Needs work » Needs review
FileSize
8.38 KB
7.82 KB

Done. Improved some comments and code.

tduong’s picture

Just a small improvement :p

miro_dietiker’s picture

Status: Needs review » Needs work

Hmm we should never attach temporary processed values to an entity. Anyone in the chain could trigger a save and thus accidentally persist the temporary value.

Instead create separate properties, variables as derivatives and pass them forward separately.

Here the intention was / is to add a $text argument to the buildXYZ methods (and yes, change their declaration!)

tduong’s picture

Status: Needs work » Needs review
FileSize
3.91 KB
11.03 KB

Alright, done.

Berdir’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

Almost there, just a bit of cleanup and comment improvements.

  1. +++ b/src/Form/JobItemForm.php
    @@ -329,31 +329,32 @@ class JobItemForm extends TmgmtFormBase {
    +        $original_text = is_array($value['translation']) ? $value['translation']['value'] : $value['translation'];
    +        $text = $original_text;
    +        // Initialise the empty translation with empty segment-only tags.
    +        if ($text == '') {
    +          $text = '';
             }
    

    As discussed, simplify to $text = ...; no $original_text needed.

  2. +++ b/src/Form/JobItemForm.php
    @@ -329,31 +329,32 @@ class JobItemForm extends TmgmtFormBase {
    +        $contexts = [$data_item, $this->entity];
    +        \Drupal::moduleHandler()->alter('tmgmt_data_item_text_input', $text, $contexts);
    +
    

    $contetx needs to use array keys ('data_item' => $data_item, 'job_item' => $this->entity)

  3. +++ b/src/Form/JobItemForm.php
    @@ -475,9 +476,17 @@ class JobItemForm extends TmgmtFormBase {
    +        // Mask HTML-tag of source's text and translation's text, if available.
    +        $source_text = $data_item['#text'];
    +        // If translation is empty, initialize it with empty segment-only tags.
    +        // @todo: Add context to show that this is a translation.
    +        $translation_text = isset($data_item['#translation']['#text']) ? $data_item['#translation']['#text'] : '';
    +        $contexts = [$data_item, $this->entity];
    +        \Drupal::moduleHandler()->alter('tmgmt_data_item_text_output', $source_text, $translation_text, $contexts);
    

    First comment: Allow other modules to change the source and translation text, for example to mask HTML tags.

    Remove the second comment, that is logic that belongs in tmgmt_ckeditor (for now).

    And the @todo can be removed.

    (the code is fine)

  4. +++ b/tmgmt.api.php
    @@ -297,3 +298,30 @@ function hook_tmgmt_job_after_request_translation(JobInterface $job) {
    + * @param array $context
    + *   An array containing $data_item and $job_item contexts.
    + */
    

    list the keys with their content. See hook_entity_view_display_alter()

tduong’s picture

Status: Needs work » Needs review
FileSize
3.69 KB
10.54 KB

Done as suggested above.

Berdir’s picture

Status: Needs review » Fixed

Thanks, now I'm happy with it. Committed.

  • Berdir committed 26f421c on 8.x-1.x authored by tduong
    Issue #2757813 by tduong: Provide support for masking HTML tags while...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.