Add HTML rule and hint+fix for missing alt attribute for tag img, applet and area

[Chris2011]

This PR adds a new HTML Rule, Hint and HintFix for a missing "alt" attribute for following tags: img, applet, area.

I copied over some code from existing ExtractInlinedStyleRule and ExtractInlinedStyleHint and checked it, how it works. Fun fact, atm ExtractInlinedStyleHintFix is not working atm, it will throw an NPE, but this is not broken due to my changes. I will try to fix this later, in a seperate PR. Please have a look into the code. I will handle this as a draft and I will change some things like to add the rule/hint to the layers.xml instead of the HtmlHintsProvider as mentioned in the comment.

Also I changed a private method which gives you the exact offset of the tag <tag></tag> or <img /> problem here is that I need to add the attribute before the closing token and the adjustOffset gave me the offset after the closing tag so I added a bool to change this behaviour just for my and future needs. The rest is still working as before.

I also wanted try to add tests for the hint, but there are atm no tests at all.

I also notied the extra space before my " alt=\"\"" when it is a closing tag. I can also try to fix this to check for token WS but it is the same behaviour as for WebStorm. It is better than nothing IMHO.

[Chris2011]

I also thought to not hardcode the supported tags, but to use the validator rule for this, but this was to much for me. If this is wanted I can try to add this too, so this will be a bit more generic if other HTML tags are coming with the same validator rule.

[Chris2011]

Now I removed the rule and hint from the HintsProvider and add it to the layer.xml. The new rule extends now HtmlRule and runs the Hint by itself.

[Chris2011]

Do I need to worry about the failing travis stuff? A quick look seems not related to my stuff anyway.

[mbien]

Do I need to worry about the failing travis stuff? A quick look seems not related to my stuff anyway.

I just restarted them. Looks good so far. You don't have a restart button?

[Chris2011]

Do I need to worry about the failing travis stuff? A quick look seems not related to my stuff anyway.

I just restarted them. Looks good so far. You don't have a restart button?

I had, but I didn't want to create some noice. But next time, I will try this first :). Thx for the info.

[Chris2011]

I left a few inline comments, but I made a general observation, that the integration looks a bit strange right now:

As you can see, I get an error indicator for the img-Tag, but I don't get the fixes. To get these I have to click inside the range of img-Tag:

I think this should be made consistent.

Thx @matthiasblaesing you are right and I already tried a bit but the offset and searching logic for tokens (Tag open, tag open symbol, tag closing, etc.) drives me crazy. Will try to fix this.

[Chris2011]

@matthiasblaesing after long thinking about this again I would disagree with the consistency. The warning/error/hint is really just for the tag img. In my little example you can see that the img tag is highlighted when a div is surrounded. So the hint shows it exactly for line + columnStart until columnEnd. For example if you have this example:

<div><img src="" /></div>

I really don't want to have the fix inside the div which is correct on this position. So the fix should be really inside of the img tag not anywhere else.

Maybe the problem here is that the hint on the sidebar has an other expectation for the user.

Here are some other things:

• I checked the HTML validator it also selects the img tag, I mean you have all the information from that.
• I checked VS Code. They have no light bulb hardcoded on the sidebar so they just underline the img tag. The light bulb comes up when I set the cursor after <|.
• I checked WebStorm, it treats the missing alt attribute also as warning, but they don't show the warning as a light bulb and I also can't switch this. The light bulb also appears just after I set the cursor on <|.

So all in all, I think how NetBeans treats and shows warnings, is not optimal but this is not what I want to fix here. I also changed from warning to suggestion on current line, but the problem here is the highlight disappears too and the "suggestion" is just seen in the statusbar BUT it just appears also on the img, not on the whitespace tokens before the tag_open_symbol. Hope that explains a bit more. I will just fix also a multi line img which makes sense to have the fix there too but not for the whole line. This will be a hole new topic IMHO.

[matthiasblaesing]

@Chris2011 something went wrong here. There is a massive set of changes where and you have conflict with master. This needs to be redone.

[Chris2011]

@matthiasblaesing thx for the info, will have a look soon.

[Chris2011]

@matthiasblaesing I think I got it now :). I did the pull and rebase from within NetBeans and it had some conflicts, that I thought I fixed them but unfortunately not correct, sry for that. I reset everything to my last commit.

So now we can focus again on the initial task and my thoughts to it.

[matthiasblaesing]

I left a few inline comments, but I made a general observation, that the integration looks a bit strange right now:

As you can see, I get an error indicator for the img-Tag, but I don't get the fixes. To get these I have to click inside the range of img-Tag:

I think this should be made consistent.

This can be ignored, as there are two hints/rules active here:

• the HTML validator marks the whole element as error
• your new rule, which indeed only applies for the elements

When I deactivate the corresponding "All Other" category, I get the desired behavior, so is ok.

[Chris2011]

@matthiasblaesing thx for your comments, will fix them :)

[matthiasblaesing]

@Chris2011 NB18 is coming nearer. Shall this stay on the NB18 milestone, then an update would be good now (branch happens in 8 days) or moving to NB 19 should be considered.

[Chris2011]

@Chris2011 NB18 is coming nearer. Shall this stay on the NB18 milestone, then an update would be good now (branch happens in 8 days) or moving to NB 19 should be considered.

We should remove the label completely. I have one bug there when it is a warning instead of a suggestion and I need to figure that out first. I had a look into my other PRs and I wanted to focus on another one first. Step by step. @matthiasblaesing thx for catching this up again :)

[matthiasblaesing]

Removed the NB18 milestone and converted this to draft.

[Chris2011]

After long round, I hopefully simplyfied it enough. I had a deeper look into existing hints like for extracting inline tag styles and seeing missing id rule and missing class rule, which has a visitor for finding this. @matthiasblaesing and @junichi11 please have a look and leave comments as necessary. Thx.

[junichi11]

Please write an example code to verify this in the description.

Please add unit tests. (CslTestBase has checkHints() and applyHint())

[Chris2011]

Please write an example code to verify this in the description.

Please add unit tests. (CslTestBase has checkHints() and applyHint())

Thx, I was searching for this. Will check it out.

[Chris2011]

Tests still missing, I'm on it. Thx for all the information.

[Chris2011]

Thx for your review @matthiasblaesing. Yes I already encountered the missing xml part but this was done by a merge because I already had it. I didn't have a merge conflict but it was removed accidentally. And indeed this is why we need tests for this. So I changed this now back to draft, because I already created the first test but it is still red due to not triggering the warning/suggestion handled as a error. After this I will change this back to normal.

Also I will have a look into the part that you wrote with the visit method change and the new added empty space. Thx.

[Chris2011]

It is still in draft mode but can please someone have a look into my test impl? @junichi11 or @matthiasblaesing. For checkHints I needed to make a change in the CslTestBase but I'm not 100% sure whether I needed to do this or not. w/o my change the problem was that the hint was never triggered due to not writing the correct property for hints/suggestions (L4387: https://github.com/apache/netbeans/pull/4485/files#diff-afbe604d62e2e9bec89e4ed0e712ad047106ed77e63c2c4030e76a54d762941fR4387).

For the applyHint test, I also needed to change code within the CslTestBase in the method findApplicableFix (L4624: https://github.com/apache/netbeans/pull/4485/files#diff-afbe604d62e2e9bec89e4ed0e712ad047106ed77e63c2c4030e76a54d762941fR4624) from text == null to text != null. I'm also not 100% sure but the code makes more sense for me now. Otherwise it will always return null which breaks the code in applyHint (assertNotNull(fix))- This test is also funny, often it fails with that the expectation of the result is not the correct one but when I debug 1min or so with F8 to jump over the code, etc. it says that the test is green and I don't know what's wrong here.

I also added a check for Error and Warning which was missing, because if Current_line_warning is false it is not that the rest is always an error or an warning. computeHints maybe is missnamed here. It will just check for html version, nothing else. So all in all the code is often very frustrating to check and I needed days for debugging and yes it could be that I don't understand the code 100% that's why I'm asking for a bit of help please. Thanks all.

[Chris2011]

Unfortunately I see that my changes break some PHP tests. Will dig into it again and would be happy to have someone that helps me break it down what I missed.

[matthiasblaesing]

My gut feeling is, that the HtmlHintsProvider and the expectations of the CslTestBase/GsfHintsManager don't match. It seems suggestions were never really supported. It might be worthy looking into JS/CSS whether there are suggestions being checked. I'm not sure, that modifying CslTestBase is really a good idea.

General comments:

1. The unstable test you observed is because you dispatch the text change into another thread (org.netbeans.modules.html.editor.hints.other.AddMissingAltAttributeHint.AddMissingAltAttributeHintFix.implement(). That (a) makes it unstable and is IMHO wrong. I suggest to adjust to this:

       @Override
       public void implement() throws Exception {
           BaseDocument document = (BaseDocument) context.getSnapshot().getSource().getDocument(true);
           document.runAtomic(() -> {
               try {
                   OffsetRange adjustedRange = HtmlTagContextUtils.adjustContextRange(document, range.getStart(), range.getEnd(), true);
                   String tagContent = document.getText(adjustedRange.getStart(), adjustedRange.getLength());
   
                   // Find last self-closing or non self-closing tag
                   Pattern closingTagPattern = Pattern.compile("(/?>)");
                   Matcher closingTagMatcher = closingTagPattern.matcher(tagContent);
   
                   if (closingTagMatcher.find()) {
                       int altInsertPosition = adjustedRange.getStart() + closingTagMatcher.start(1);
   
                       // Check whether a space before alt is needed or not
                       boolean needsSpaceBefore = altInsertPosition == 0 || tagContent.charAt(closingTagMatcher.start(1) - 1) != ' ';
                       boolean isSelfClosing = closingTagMatcher.group(0).endsWith("/>");
                       String altAttribute = (needsSpaceBefore ? " " : "") + "alt=\"\"" + (isSelfClosing ? " " : ""); // NOI18N
   
                       document.insertString(altInsertPosition, altAttribute, null);
                   }
               } catch (BadLocationException ex) {
                   // Ignore
               }
           });
       }

   There is no need to do this on the EDT. The locking infrastructure of the documents should handle this.

2. The null check in CslTestBase#findApplicableFix begins to make sense, when you write it like this:

   

   The only difference compared to master is in line 4603, 4621 is only modified when comparing with your version.

   Then org.netbeans.modules.csl.api.test.CslTestBase.applyHint(NbTestCase, Rule, String, String, String) can be called with null as final parameter, which would invoke the found fix without checking for the description.

3. Merges make it hard to compare against the base revision. Until this is changed incrementally/being reviewed I suggest to either not merge master or use rebases to cleanup/update.

[Chris2011]

My gut feeling is, that the HtmlHintsProvider and the expectations of the CslTestBase/GsfHintsManager don't match. It seems suggestions were never really supported. It might be worthy looking into JS/CSS whether there are suggestions being checked. I'm not sure, that modifying CslTestBase is really a good idea.

Hey @matthiasblaesing thx for your info and hint. I'm definitely with you, I will check the JS stuff, because CSS has also no real hints/fixes (just basic stuff) as I have. I also thought that this was never tried before. Lemme check more what I can do.

For the invokeLater, thx for the hint with the change, I got this from ExtractInlinedStyleHint but due to I not doing extrem things like opening a new UI, it is not needed here. I just add stuff into the editor.

[Chris2011]

@matthiasblaesing so another research here:

• There are no CSS Hints/Suggestions/Fixes, etc, just the CssHintsProvider where the computeHints, computeSuggestions and computeSelectionHints method bodies are empty. I already have one CSS hint/suggestion on another branch, just to test smth but without any test.

• The best implementation that I found is the PHP stuff for suggestions, but the problem is that I get a timeout when I try to run a PHP related suggestion test like this one locally: https://github.com/apache/netbeans/blob/master/php/php.editor/test/unit/src/org/netbeans/modules/php/editor/verification/InitializeFieldsSuggestionTest.java and on my branch, on the pipeline, the tests are not executed/skipped, but maybe because of the failing latte tests.

• I also had a look into JsHintsProvider and JsHints. There is just one fix, changing the EcmaLevel but everything seems a warning. The tests will not work with suggestions. There will be a Parse exception, when I change the severity warning to a current_line_waning. Ok I really don't know whether I need to change smth else.

Fazit: It seems PHP and maybe JS are better implemented in their *HintsProvider and I need to change the whole logik of the HtmlHintsProvider but JS also lacks of tests and suggestions are also not a big player for JS (not yet). Should this be part of this ticket? I guess not.

I also think that we should make this somehow consistent for all CSL languages as good as possible. PHP uses Classes for like SuggestionRule, HintRule and ErrorRule, which just set the severity to the correct one and so on. So probably I will create another PR with the "fx" of the HtmlHintsProvider with using Tests. I will ake the refactoring in this PR because I already have a Suggestion here.

I think I ran into a rabbithole and yes, it is not your business but the code is a big mess and I want to rework it and hope to not break more than before. Any other thoughts/suggestions/hints? :D