RFR: 8223378: CSS solution for navbar problem with <'a> elements is not ideal

Fri Nov 22 01:20:22 UTC 2019

Before getting into the code details, I will say how much I like the 
resulting UI result!

On 11/14/2019 09:04 AM, Hannes Wallnöfer wrote:
> I agree, calling something *Content that doesn’t extend Content is not great.

>
> I actually considered „Body“ and „BodyBuilder“, but the former doesn’t fit because the generated content does not include the <body> tag (which is generated elsewhere and dependent on page type), and the latter has that annoying connotation of people in undies flexing their muscles.

Yeah, I had the same issue with "BodyBuilder" :-)  As for "Body", my 
initial reaction is, why isn't this a builder for the <body> tag?
>
> What do you think about BodyContents (plural) or BodyContentBuilder?

For now, I guess I'd suggest to go with BodyContents, but I do like the 
idea that maybe down the road, we evolve this into Body, and have it 
take care of the complete overall high level structure of the page, as 
represented by a <body> tag. There's not much else involved ... see 
HtmlDocletWriter.getBody.  And now that frames have gone, I'm not sure 
we need scripts to set the window title, etc.   Also, I note that unless 
we separately choose to get rid of the script stuff, evolving 
BodyContents into Body should just be a code refactoring, without 
related changes to any tests, which would significantly reduce the cost 
of the refactoring.

>
> Regarding the unused fluency: yes, it’s superfluous and should probably be removed.
I guess I like using fluency when it is reasonable to do so, so I would 
tend to go the other way: retain it and use it.

-------

In addition,

I think its wrong to move the "top" content into the Navigation class. 
It doesn't help that the Navigation class is not well designed in the 
first place and needs to be overhauled anyway. Instead of moving "top" 
into Navigation, I think there are a couple of possibilities.

1. Add a new Header class for the header of the page, containing the top 
text and the content of the Navigation object.

2. #1 may be overkill, in which case you could extend/generalize your 
new BodyContent(s) class so that instead of providind 
"setHeader"/"setFooter", you provide "addHeaderContent", 
"addFooterContent".  Then the code to "addTop" into the Navigation 
object becomes a pair of calls to add the top into the BodyContents 
header, followed by the contents of the Navigation object.

3. Create a ContentBuilder.  Call "addTop" to put the top content into 
that content builder, then add Navigation.toContent() into the content 
builder, and finally set the content builder  as the header of the 
bodyContents.   Of these 3 suggestions, I think I recommend this one.

What's wrong with Navigation? IMO, it is broken in a number of ways. 
(These are general criticisms, mostly not specific to your changes.)

a. It's too high level to be a mid-level builder in the formats.html 
package.  There's way too much use of "configuration", and way too much 
page-specific knowledge, embedded in the use of PageMode and switch 
statements performing different actions based on the PageMode.    
Instead, it should provide lower-level building blocks, and the 
page-specific behavior should be replaced by (fluent?) calls to 
customize the properties when the page-specific instance is created.  
Most uses of "configuration" as well as the PageMode enum could/should 
be eliminated from the abstraction.

b. The class normally generates a single <nav> element. IMO this is an 
abuse of <nav> which should just be a container for a series of links. 
As the code stands now, not only does it contain all the links, but it 
also contains title text, the search box, and (with your change) the top 
text.  At some point, it should be rewritten to generate a <div>, which 
may in turn contain <nav> elements surrounding the lists of navigation 
links.  You could persuade me that we need a different/better name, but 
given it is used for both the header and footer, neither `Header` nor 
`Footer` is a reasonable name. `Banner` maybe?

c. There is silliness in the code.  Look at the code-flow in 
Navigation.toContent.

     Content contentTree =new ContentBuilder();
     if (!configuration.nonavbar) {
	// compute tree
         return tree;
     }
     return contentTree;

This could be simplified/improved.

d. By moving the "top" text into Navigation, you have made it subject to 
the `-nonavbar` option, embodied by `configuration.nonavbar`. That's an 
inappropriate change in behavior. The top text is typically used for 
legal info and should not be hidden if `-nonavbar` is specified.

-----

Should the various declarations of bodyContent in the various subtypes 
of HtmlDocletWriter be replaced by a single protected declaration in 
HtmlDocletWriter itself?

-----

I noticed the following on PackageSummaryWriter/PackageWriterImpl, but 
there may be equivalent instances on other pages as well.  Look at the 
changes towards the end of the file, line 295, 307.
The parameter "contentTree" is now unused.  You've changed the semantics 
of the method from "add the 2nd argument to the 1st argument" to "add 
the 2nd argument to a class member". That in itself is not wrong, but it 
is not the semantics documented in the overriden method.  One way or 
another, you should align the spec/impl of the super method and the impl 
method.  That being said, I think this ties into some similar related 
issues, with respect to other add... methods and the use of the 
page-global bodyContent member.

-----

I noticed this on SerializedFormWriterImpl.java; it may apply to other 
pages as well.  You've changed the impl of printDocument, which is 
supposed to (just) print the tree that is provided as an argument.  It 
seems wrong to be adding in bodyContent at this point, and related to 
the previous comment about adding nodes into page-global variables 
instead of the given arguments.

----

OK, now I see a root cause of the problems and why you have done some of 
what you have done.  I see that the issue is the historical split 
between the format-neutral toolkit world and the formats.html world ... 
and that `BodyContent` is a builder but not a subtype of Content.  The 
problem is exacerbated somewhat by the inconsistent asymmetry between 
the use of `get` methods that return Content and `add` methods that take 
a Content as an argument.

*Temporary Summary:*

I don't have a good solution/suggestion at this point. I need to think 
about this. I'm sure we don't want to get into an big refactoring of the 
Builder/Writer/Impl APIs, but equally, it seems difficult to conform to 
those APIs as specified.

That being said, I'm beginning to see a way forward in which the API is 
biased more towards to imperative `void addXyx(...)` style, at least at 
the outer levels of the page.  Taking PackageWriterImpl as an example, 
look at getPackageHeader.  It too does not really conform to its 
declared specification.  It creates a mostly empty `bodyTree`, then 
stashes most everything in member variables for `navBar` and 
`bodyContent` and finally returns the empty `bodyTree` ...   which is 
not the package header implied by the name of the method ... and then 
eventually, there's a second sleight-of-hand when the `bodyContent` is 
surreptiously slipped into the mostly empty body at the last minute in 
`printDocument`.  Perhaps we should really go "all-in" on side-effect 
methodology and have *WriterImpl just build up <body>/bodyContent/navBar 
in member variables. In other words, use one instance of (a subtype of) 
HtmlDocletWriter per  page, which we already do, building up member 
variables for the page, resulting in a final parameter-less call of 
`printDocument()` when the content of the page has finally been prepared.

-- Jon

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/javadoc-dev/attachments/20191121/304a07d5/attachment.html>