Replace jQuery $().toggleClass by vanilla js classList.toggle in sap.f library

[mauriciolauffer]

Just replacing jQuery .toggleClass by vanilla js classList.toggle in sap.f library. Hopefully more people can help tackling other libraries, then other jQuery features, until we don't have jQuery in the code base anymore.

jQuery .toggleClass can be replaced by classList.toggle and it's supported by IE11, not just modern browsers.

sap.f.DynamicPageTitle still have 2 references to .toggleClass due to method ._setContentAreaFlexBasis using parameter $node, it's not locally defined.

https://github.com/mauriciolauffer/openui5/blob/6d0303f509537763dd321a3d4f48a10be23bf998/src/sap.f/src/sap/f/DynamicPageTitle.js#L1441
https://github.com/mauriciolauffer/openui5/blob/6d0303f509537763dd321a3d4f48a10be23bf998/src/sap.f/src/sap/f/DynamicPageTitle.js#L1445

[codeworrior]

Just some thoughts...

[codeworrior]

This is IMO not a good pattern to detect child elements. Not using the full ID has a certain risk of finding the wrong element.

Imagine a DOM like this

<div id="x">
	<div id="x-children">
		<div id="chart-container">
		</div>
	</div>
	<div id="x-container">
	</div>
</div>

document.getElementById("x").querySelector("[id$='-container']") will return the <div id="chart-container">.
This might not apply to DynamicPage as the suffix contentFitContainer is quite specific and nesting is not likely. But I would prefer a consistent approach across controls to make code reading easier for maintainers.

[mauriciolauffer]

This is pretty much the same thing as is today with jQuery. Have a look at how this.$contentFitContainer is set on line 1792:

this.$contentFitContainer = this.$("contentFitContainer");

Today, jQuery looks for any child id which contains "contentFitContainer" (at any position, it may not be a suffix) with this as the parent. The change doesn't use contains, it ensures child id ends with "contentFitContainer", it has to be a suffix. The query is always performed in the controller context making it very specific and reducing the risk of getting unexpected results. The new approach is safer than the current one.

[codeworrior]

Sorry, it isn't the same and definitely not safer. this.$("contentFitContainer") translates to

  jQuery(document.getElementById(this.getId() + "-" + "contentFitContainer"));

which looks for a very specific ID. See Element.js#L423-460.

querySelector returns the first DOM element in document order that matches the selector. And as sketched above, this is NOT safe with a pure suffix search (in the general case).

And I would even expect the querySelector to be less efficient. getElementById is (hopefully) a simple lookup, whereas querySelector has to search and match DOM elements. Can be optimised by the browser, too. But I wouldn't bet on it that every possible kind of selector has been optimised.

[mauriciolauffer]

Oh, my bad! I haven't seen Element.prototype.$ implementation before. I thought it was a regular jQuery object, no idea it was calling this.getDomRef. So, for these cases, just executing this.getDomRef('someSuffix') should suffice as it'd get by full ID and not return a jQuery object.

Performance wise, getElementById is ~2x faster than querySelector. However, it doesn't mean querySelector is slow, it executes +2M ops/sec.
https://www.measurethat.net/Benchmarks/Show/2488/0/getelementbyid-vs-queryselector

[codeworrior]

Comparing the size of before and after code, I think we still should consider using some convenience. Something much closer to the vanilla DOM APIs, but not as verbose.

[mauriciolauffer]

If jQuery doesn't find any element, calling .toggleClass won't throw an error, it won't break the app. The same doesn't apply for vanilla DOM APIs. If domRef is null .querySelector will throw an error. If .querySelector result is null, .classList.toggle will throw an error. That's why all the validations.

An utility function could be created to make it easier to use. Something like:

function toggleClass(contextDomRef, query, cssClass, condition) {
  if (!contextDomRef || !query || !cssClass) {
    return;
  }
  const selectedElement = contextDomRef.querySelector(query);
  if (!selectedElement) {
    return;
  }
  if (typeof condition === 'boolean') {
    selectedElement.classList.toggle(cssClass, condition);
  } else {
    selectedElement.classList.toggle(cssClass);
  }
}

Usage would be like:

toggleClass(document.body, 'div', 'someCssClass');
toggleClass(this.getDomRef(), '[id$="some-id-suffix"]', 'someCssClass', 1 > 0);

[codeworrior]

That was my point, exactly. I'm in favour of replacing or accompanying our Element.prototype.$ with a more lightweight substitute, based on vanilla JS. The classList mutations are only one example where this might make sense.

Some months ago, we had made experiments with such a substitute. Roughly 95% of the immediate calls to the object returned by Element.prototype.$ could be easily substituted with vanilla JS. The remaining 5%, however, make a rollout difficult. Replacing Element.prototype.$ most likely would cause compatibility issues, so accompanying it with a similar API, limited to what can be backed by vanilla JS, makes the most sense to me.

We've however postponed activities reg. such a substitute until we have a plan to remove jQuery completely. Other areas where we use it, namely event handling, are more tricky to replace (as long as compatibility limits us).

These considerations don't prevent the removal of existing jQuery usages, for sure. We also have done it here and there. It just has to be decided case-by-case.

[mauriciolauffer]

I think the main issue would be when Element.prototype.$ doesn't find anything. Today it always returns a jQuery object and one can call any jQuery method from it, even when this.getDomRef() returns null.

With vanilla js, it could cause problems if Element.prototype.$ doesn't return a jQuery object and returns null. One'd always need to check the returned object before executing any method. In my PR, I've added checks for that reason. Sometimes this.getDomRef() or querySelector would return null and cause issues.

[cla-assistant]

All committers have signed the CLA.

[flovogt]

@mauriciolauffer Thanks for this PR. I have created an internal incident DINC0034895 to follow-up on this. The responsible team will get in touch with you.

[IlianaB]

Hello @mauriciolauffer ,
Thank you for your pull request.

It seems it's been a while, since the pull request has been opened. As it is outdated, the change needs to be rebased with the master branch.

What is more, we would like to ask you for some rework.
Accessing the DOM too frequently or unnecessarily (each time we are searching for an element), triggers a reflow or repaint, which are expensive operations. We are trying to minimize accessing the DOM, whenever possible, in order to keep a good performance.
As you have probably noticed, Dynamic page, for example, has _cacheDomElements function (called after rerendering), which caches most of the DOM references, so we do not need to search for them all the time. We would like to keep it like this and work as much as possible with cached DOM references.
That's why, in order this pull request to be merged, additional changes must be done - wherever jQuery methods are used on the saved DOM references (not jQuery objects), they need to be replaced with vanilla JavaScript.

Regards,
Iliana

[viktorsperling]

After a short conversation with @IlianaB we have decided to close this PR since, there is no response from the author for more than 4 weeks.

[mauriciolauffer]

"Accessing the DOM too frequently or unnecessarily (each time we are searching for an element), triggers a reflow or repaint, which are expensive operations."
That's incorrect. Reading the DOM doesn't trigger reflow/repaint. Updating the DOM does.